Repository navigation
refactor(validators): stop claiming seconds for millisecond-based params - #518
Open
maulanawirawan wants to merge 1 commit into
Open
maulanawirawan wants to merge 1 commit into
maulanawirawan wants to merge 1 commit into
Conversation
PlaywrightConfig.timeout (default 30000) and wait are consumed as milliseconds by Playwright (set_default_navigation_timeout, wait_for_timeout), while retry_delay on the same alias really is seconds (time.sleep). All three shared one alias named 'Seconds', so the annotation stated wrong units for timeout/wait — e.g. a user passing timeout=60 following the static Fetcher convention (seconds) gets a 60 ms navigation timeout. Introduce 'Milliseconds' and use it for timeout/wait in PlaywrightConfig and _fetch_params; retry_delay stays on 'Seconds'. Behavior unchanged: both aliases carry the identical Annotated[float, Meta(ge=0)] constraint. AI disclosure: this patch was written primarily with Claude Code assistance (analysis + code), reviewed and submitted by me.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In
scrapling/engines/_browsers/_validators.py, one type alias namedSecondsis used for parameters that are consumed in two different units:timeoutSeconds = 30000set_default_navigation_timeout()/page.goto()(Playwright, ms)waitSeconds = 0page.wait_for_timeout(params.wait)(_stealth.py)retry_delaySeconds = 1time_sleep(self._config.retry_delay)(_stealth.py)So the annotation stated wrong units for
timeoutandwait(30000 "Seconds" ≈ 8.3 hours), andwait=2reads like 2 seconds but waits 2 ms.The ms behavior itself is documented — the problem is only that the shared alias name contradicts it. This bit me in practice: passing
timeout=60while following the staticFetcherconvention (timeoutis documented in seconds there) yieldsPage.goto: Timeout 60ms exceededinstantly.What this PR does
Introduces a
Millisecondsalias and uses it fortimeout/waitinPlaywrightConfig(and the_fetch_paramsdataclass).retry_delaystays onSeconds.Annotated[float, Meta(ge=0)]msgspec constraint — validation, defaults, and everything downstream are untouched. This is purely making the type annotation tell the truth.Fetcher.timeoutis documented in seconds vs browser fetchers in ms) would be a breaking change right after v0.5 — happy to open a discussion if you'd rather go that way, but the alias correction stands on its own either way.Validation
ruff checkclean.AI disclosure (per AI_POLICY.md)
This patch was written primarily with Claude Code assistance (analysis + code); reviewed and submitted by me.