Skip to content

Refactor Slice B exception handlers to use specific Playwright errors (#46) - #130

Merged
LeyckerS merged 2 commits into
LeyckerS:mainfrom
AdvaitVarhade:refactor/exception-handlers-slice-b
Aug 2, 2026
Merged

Refactor Slice B exception handlers to use specific Playwright errors (#46)#130
LeyckerS merged 2 commits into
LeyckerS:mainfrom
AdvaitVarhade:refactor/exception-handlers-slice-b

Conversation

@AdvaitVarhade

Copy link
Copy Markdown
Contributor
  • Narrowed broad except Exception: blocks in lines 653-943 of moon_extract.py.
  • Locally imported PlaywrightError and PlaywrightTimeoutError inside on_route and _extract_datanodes_on_context to preserve deferred Playwright initialization.
  • Replaced broad swallows in popups/page close finally blocks with (PlaywrightError, Exception).
  • Updated _cdp_alive to catch specific urllib/JSON errors instead of a broad Exception.

…LeyckerS#46)

- Narrowed broad `except Exception:` blocks in lines 653-943 of `moon_extract.py`.
- Locally imported `PlaywrightError` and `PlaywrightTimeoutError` inside `on_route` and `_extract_datanodes_on_context` to preserve deferred Playwright initialization.
- Replaced broad swallows in popups/page close `finally` blocks with `(PlaywrightError, Exception)`.
- Updated `_cdp_alive` to catch specific urllib/JSON errors instead of a broad Exception.

@LeyckerS LeyckerS left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slice B is the last one and I want it in, but four of these narrowings do not narrow anything. Small fix, then it merges.

The blocking part

except (PlaywrightError, Exception):

PlaywrightError subclasses Exception, so that tuple catches exactly what except Exception: catches on its own. Verified rather than assumed:

>>> issubclass(playwright.async_api.Error, Exception)
True

It appears four times — on_route, the ad-popup close in the sweep loop, and both closes in the finally. Each one reads as a narrowing to anyone skimming the file, and none of them is one. That is worse than leaving except Exception: alone, because the next person will believe the handler is bounded when it is not.

Both options are fine, pick per site:

  • Genuinely narrow itexcept PlaywrightError: where a Playwright error is all that can realistically occur. Closing an already-closed page is exactly that case.
  • Keep it broad and say whyexcept Exception: with a one-line reason. That is what #54 asked for and it is a legitimate answer.

What is not an option is a tuple that looks like the first and behaves like the second.

Two smaller things while you are in there

urllib.error is not imported. Line 970 catches urllib.error.URLError, and the file only has import urllib.request (line 875). It works today purely because urllib.request imports urllib.error internally — an implementation detail, not a guarantee, and CI cannot catch it because the tuple is only evaluated when the exception fires. Add import urllib.error next to it. The rest of that handler is the best narrowing in the PR: (URLError, OSError, JSONDecodeError, KeyError) is precisely what probing a dead CDP port can raise.

The import inside on_route is redundant. on_route is nested inside _extract_datanodes_on_context, which already imports PlaywrightError at the top, so the closure picks it up. Harmless, just noise.

What is right

(PlaywrightError, PlaywrightTimeoutError) on wait_for_load_state and reload is correct — TimeoutError does subclass Error, so it is technically redundant too, but there it documents the two distinct failures a caller should expect rather than pretending to exclude something. I would keep those.

Keeping the import at function level rather than hoisting it is right, and you know why — you established it in #82.

Fix the four tuples and add the urllib.error import and this goes in. You have done three of the four slices now; it would be good to have the set closed by the same person who started it.

@LeyckerS LeyckerS left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All four fixed, and this closes #46 entirely. Merging.

except PlaywrightError: on the four sites that were tuples — now genuinely bounded rather than except Exception wearing a costume. import urllib.error is in, so the _cdp_alive handler no longer depends on urllib.request happening to pull it in. And you dropped the redundant nested import in on_route without being asked twice.

Keeping (PlaywrightError, PlaywrightTimeoutError) on wait_for_load_state and reload was the right call. It is technically redundant — TimeoutError subclasses Error — but there it names the two distinct outcomes a reader should expect, which is documentation rather than decoration. The distinction between that and the four I sent back is exactly the one worth having.

_cdp_alive is the best handler in the set: (URLError, OSError, JSONDecodeError, KeyError) is precisely what probing a dead CDP port can raise, and each of the four is reachable.

CI green on all five Python versions, including the 3.13 and 3.14 that #129 added an hour ago.

#46 is now complete — 30 handlers, four slices, three contributors:

Slice
A moon_extract.py 71–556 #82 (@AdvaitVarhade)
B moon_extract.py 653–943 #130 (@AdvaitVarhade)
C moon_extract.py 1025–1249 #118 (@AashishGupta2007)
D moon_engine.py #71 (@Moferanoluwa)

You opened it and you closed it. Four merged PRs from you now.

One thing not worth a round trip: pass # comment uses one space before the # where PEP 8 asks for two. Ruff cannot see it — E261 is a whitespace rule and those only run under preview mode, which ruff.toml does not enable. Worth knowing that the linter is quieter than it looks, rather than worth changing now.

@LeyckerS
LeyckerS merged commit 047367b into LeyckerS:main Aug 2, 2026
7 checks passed
LeyckerS added a commit that referenced this pull request Aug 2, 2026
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.

2 participants