-
Notifications
You must be signed in to change notification settings - Fork 438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use a browser to load the page #481
base: main
Are you sure you want to change the base?
Conversation
chore: fix lints chore: fix lints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @IMXEren - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟡 Security: 1 issue found
- 🟡 Testing: 8 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
This comment was marked as off-topic.
This comment was marked as off-topic.
Pretty big PR. I'm unfamiliar with the headless browser too. So, I'm not sure I'll be able to maintain, in case any issue occurs. I'll mark the PR as draft for now. Unless i get confident that I can maintain it myself. Plus docker image size difference is huge. |
It's alright. |
It's unnecessary to setup dependencies again & again, rather raise `ValueError`.
Quality Gate passedIssues Measures |
Let me know what you think about this...
Fixes #375
There is a possible disadvantage for local docker users, i.e. the uncompressed size is 3x times (1.47 GB) of the current image (500 MB). I suppose offerring this feature as another tag can be a good option.