Skip to content

Low-hanging improvements to the crawler APIs - #698

Merged
jodal merged 11 commits into
mainfrom
crawler-api-cleanups
Sep 4, 2026
Merged

jodal merged 11 commits into
mainfrom
crawler-api-cleanups

Conversation

@jodal

@jodal jodal commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Low-hanging improvements to the crawler APIs, found by reading LxmlParser,
FeedParser and all 60 hand-written crawl() implementations. One commit per
change, so they can be reviewed or dropped individually.

Dead code and wrong types in LxmlParser

  • DoesNotExist was raised nowhere, so the handler wrapping _get_all() could
    never run. Both are gone.
  • _decode() could never take its bytes branch, as lxml returns str from both
    Element.get() and text_content(). That it was applied in _get_all() but
    not _get_one() showed nothing depended on it.
  • text() was annotated as returning list[str] | str | None, while its
    overloads and _get_one() only ever produce str | None.

Errors that were classified wrong

  • The three shared base crawlers asserted the parser had found an image URL,
    preempting the ImageURLNotFound that add_image() raises. That turned a
    CrawlerBroken naming the comic and date into a bare AssertionError in the
    catch-all, and would vanish under python -O.
  • LxmlParser fetched pages without checking the status, so an error page was
    parsed as if it were the comic's page. Finding no image in it, a crawler
    reported "no release found" — which is what a day the comic did not publish on
    looks like — so a site that moved or started refusing us stayed invisible.
  • Three crawlers that already establish the entry is the comic, by its link,
    tags or publishing date, skipped to the next entry when the image selector
    then found nothing. They now let the empty URL reach CrawlerImage, which
    reports ImageURLNotFound at error level.

Two additions that shrink the crawlers

  • first=True on the singular accessors takes the first match in document order
    instead of raising MultipleElementsReturned. Pages that legitimately match
    several had to fall back to the plural accessor and index into it, guarding the
    empty list by hand. Seven crawlers move over.

  • element() / elements() return parsers scoped to the matching elements, for
    crawlers that need to pick a container and then read its children. Four
    crawlers come off page.root, where they hand-rolled the extraction and lost
    the default handling, the multiple-match guard and the CSS selectors. This also
    fixes a latent bug in the Evil Inc crawler, which tested xpath() against
    None while it returns an empty list.

    Awkward Zombie and Subnormality keep using page.root: they match on element
    text and sort by a parsed style attribute, which the selector API does not
    express.

Smaller things

  • Entry.title and Entry.link reach crawlers through __getattr__, so they
    typed as Any and nothing checked their use in the 41 feed crawlers. Declared
    alongside the existing summary and content0 annotations.
  • The OOTS crawler called feed.all() twice, rebuilding the entry list to test
    for and then take the first entry.
  • history_length_days = 0 resolves to the same history_start as leaving both
    history attributes out, which already means only today can be crawled.

Worth watching after deploy

raise_for_status() is the one change that can turn a working crawler into a
failing one, if a site serves usable content with a non-2xx status. I checked the
crawlers that mention HTTP status: Penny Arcade's soft-404 comes with a 200 and
is unaffected, Dumbing of Age's 404 note is about the image server on the
downloader path, and Buttersafe's User-Agent dodges a 403 that would now surface
as a warning instead of being parsed silently.

Not touched, but the same shape

  • Wumo's guard is now safe to drop too, since a gone page raises rather than
    falling through.
  • CreatorsCrawlerBase also calls httpx.get() without a status check and then
    .json(), so a non-2xx fails as a JSON decode error and lands in the catch-all
    rather than as a CrawlerHTTPError.

LxmlParser and FeedParser have no tests, so the new first=True and
elements() surface ships unit-tested only by hand against sample markup.

https://claude.ai/code/session_013DLpoXp8z36AGE2ksWG58n

@codecov

codecov Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 36.11111% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.00%. Comparing base (81b4208) to head (e1c8f66).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/comics/aggregator/lxmlparser.py 32.35% 23 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #698      +/-   ##
==========================================
+ Coverage   68.97%   69.00%   +0.02%     
==========================================
  Files          46       46              
  Lines        2069     2071       +2     
==========================================
+ Hits         1427     1429       +2     
  Misses        642      642              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jodal added 11 commits September 4, 2026 12:35
The three shared base crawlers asserted that the parser had found an
image URL. CrawlerImage.url is deliberately optional, and add_image()
validates it into an ImageURLNotFound, which is classified as
CrawlerBroken and logged as an error naming the comic and date. The
assert preempted that with a bare AssertionError, which fell through to
the catch-all handler, and would vanish entirely under python -O.
Nothing in the codebase raises DoesNotExist, so the handler wrapping
_get_all() could never run. Both the handler and the exception class go.
lxml returns str from both Element.get() and text_content(), so the
bytes branch could never run. That it was applied in _get_all() but not
in _get_one() showed nothing depended on it either way.
The implementation was annotated as returning list[str] | str | None,
while both overloads and _get_one() only ever produce str | None.
The singular accessors raise MultipleElementsReturned when a selector
matches more than one element. Pages that legitimately match several,
where the comic is the first one, had to fall back to the plural
accessor and index into it, guarding against the empty list by hand.
first=True takes the first match in document order instead of raising,
so those call sites collapse to the ordinary singular form.
Crawlers that needed to pick a container and then read its children had
to reach past the parser into page.root and hand-roll the extraction,
losing the default handling, the multiple-match guard and the CSS
selectors. element() and elements() return parsers scoped to the
matching elements, so the same extraction API keeps working one level
down. Selectors on a scoped parser match the element itself as well as
its descendants, since cssselect scopes them as descendant-or-self.

Four crawlers move off page.root. This also fixes a latent bug in the
Evil Inc crawler, which checked the result of xpath() against None,
while xpath() returns an empty list when nothing matches.

Awkward Zombie and Subnormality keep using page.root: they match on
element text and sort by a parsed style attribute, which the selector
API does not express.
link and title reach crawlers through __getattr__, so they typed as Any
and nothing checked their use in the 41 feed crawlers. Declaring them
alongside the existing summary and content0 annotations types them
without changing lookup, as a bare annotation creates no class
attribute.
feed.all() rebuilt the whole entry list on each call, and the crawler
called it twice to test for and then take the first entry.
A zero-day history resolves to the same history_start as leaving both
history attributes out, which already means only today can be crawled.
…s known

These three crawlers already establish that the entry is the comic, by
its link, its tags or its publishing date. Skipping to the next entry
when the image selector then finds nothing reported the crawl as 'no
release found' at info level, which is the answer for a day the comic
did not publish on, not for a page whose markup moved. Letting the empty
URL reach CrawlerImage raises ImageURLNotFound instead, naming the comic
and the date at error level.

The first of the two guards in the Joy of Tech crawler is the fallback
between its two selectors, and stays.
LxmlParser fetched pages without checking the status, so an error page
was parsed as if it were the comic's page. Finding no image in it, a
crawler reported 'no release found', which is what a day the comic did
not publish on looks like, and a site that moved or started refusing us
stayed invisible.

httpx.HTTPStatusError is an httpx.HTTPError, which get_release() already
wraps into CrawlerHTTPError, so a gone page now reports as a transient
failure while a page we did fetch but could not read reports as a broken
crawler.
@jodal
jodal force-pushed the crawler-api-cleanups branch from 73fe356 to e1c8f66 Compare September 4, 2026 10:35
@jodal
jodal merged commit adb88dd into main Sep 4, 2026
8 checks passed
@jodal
jodal deleted the crawler-api-cleanups branch September 4, 2026 10:35
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