Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
collindutter
left a comment
collindutter
left a comment
There was a problem hiding this comment.
Thanks! Can you please add some unit tests?
|
Added unit tests for the new options: default chromium + headless, executable_path passthrough, and firefox selection. All green locally (12 passed). Thanks for the review. |
|
@collindutter friendly bump whenever you have a moment - the unit tests you asked for are in (default chromium + headless, executable_path passthrough, firefox selection; all green locally). No rush, just flagging it is ready for another look. |
collindutter
left a comment
collindutter
left a comment
There was a problem hiding this comment.
Minor feedback, otherwise looks good!
| exclude_classes: list[str] = field(default=Factory(list), kw_only=True) | ||
| exclude_ids: list[str] = field(default=Factory(list), kw_only=True) | ||
| timeout: int | None = field(default=None, kw_only=True) | ||
| browser_type: str = field(default="chromium", kw_only=True) |
There was a problem hiding this comment.
Can we add some validation here? Maybe replace str with Literal["chromium", "firefox", "webkit"] and add an attr's validator validator=attrs.validators.in_(["chromium", "firefox", "webkit"])?
|
@collindutter friendly nudge on this one, still ready to merge whenever you get a chance to look. Let me know if anything else needs changing. |
@feder-cr did you see my comment here? Please put the PR out of draft status and re-request my review when it's ready for review. |
2 participants
fetch_url was hardcoded to p.chromium.launch(headless=True), so there was no way to point it at a different engine or a custom binary. Adds browser_type (defaults to 'chromium', unchanged behavior) and executable_path fields, following the attrs field pattern already used here. Motivated by invisible_playwright, a Playwright wrapper around a Firefox build patched at the source level for a realistic fingerprint, useful for scrapes that hit bot detection on the default Chromium path. Opened as draft, happy to adjust naming or add a test.