Skip to content

refactor(validators): stop claiming seconds for millisecond-based params - #518

Open
maulanawirawan wants to merge 1 commit into
D4Vinci:devfrom
maulanawirawan:fix/seconds-type-alias
Open

maulanawirawan wants to merge 1 commit into
D4Vinci:devfrom
maulanawirawan:fix/seconds-type-alias

Conversation

@maulanawirawan

Copy link
Copy Markdown

Summary

In scrapling/engines/_browsers/_validators.py, one type alias named Seconds is used for parameters that are consumed in two different units:

param annotation (before) actual unit consumption docs
timeout Seconds = 30000 milliseconds set_default_navigation_timeout() / page.goto() (Playwright, ms) docs/fetching/stealthy.md: "The timeout (milliseconds)… default is 30,000 ms"
wait Seconds = 0 milliseconds page.wait_for_timeout(params.wait) (_stealth.py) docs/fetching/stealthy.md: "The time (milliseconds)"
retry_delay Seconds = 1 seconds time_sleep(self._config.retry_delay) (_stealth.py) docs/fetching/stealthy.md: "Seconds to wait between retry attempts"

So the annotation stated wrong units for timeout and wait (30000 "Seconds" ≈ 8.3 hours), and wait=2 reads 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=60 while following the static Fetcher convention (timeout is documented in seconds there) yields Page.goto: Timeout 60ms exceeded instantly.

What this PR does

Introduces a Milliseconds alias and uses it for timeout / wait in PlaywrightConfig (and the _fetch_params dataclass). retry_delay stays on Seconds.

  • No behavior change: both aliases carry the identical Annotated[float, Meta(ge=0)] msgspec constraint — validation, defaults, and everything downstream are untouched. This is purely making the type annotation tell the truth.
  • Cross-fetcher unit unification (static Fetcher.timeout is 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 check clean.
  • Test suite locally (Windows, dev @ e5d1e36): 2210 passed; the 10 failures I saw fail identically on clean dev without this patch (local env: shell/session-state tests) — so zero regressions from this change.

AI disclosure (per AI_POLICY.md)

This patch was written primarily with Claude Code assistance (analysis + code); reviewed and submitted by me.

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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant