Skip to content

Handle NewsScraper transport errors - #98

Merged
vedaant00 merged 1 commit into
mldsveda:mainfrom
shard872:agent/handle-news-fetch-errors
Aug 5, 2026
Merged

Handle NewsScraper transport errors#98
vedaant00 merged 1 commit into
mldsveda:mainfrom
shard872:agent/handle-news-fetch-errors

Conversation

@shard872

@shard872 shard872 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • convert NewsScraper transport failures into structured ScrapeResult errors
  • cover feed, site, and article paths in both sync and async APIs
  • keep HTML/XML parsing outside the transport guards so programming defects still propagate

Why

NewsScraper was the outlier among the API scrapers: failed HTTP requests escaped as exceptions and could crash callers instead of returning the library's normal ScrapeError representation. The article path also required separating fetch from parse so the fix would not hide parser failures.

Impact

Callers now receive empty data plus one ScrapeError containing the failed URL and transport message. Successful scraping, route priority, invalid XML behavior, and public method signatures are unchanged.

Closes #87.

Verification

  • PYTHONPATH=src python3 -m pytest tests/test_scrapers/test_news.py -q — 21 passed
  • PYTHONPATH=src python3 -m pytest tests/ -q — 384 passed, 5 skipped, 19 deselected
  • ruff check src/ — all checks passed
  • git diff --check — clean

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates NewsScraper to align with the library’s other scrapers by converting HTTP/transport failures into a structured ScrapeResult containing a single ScrapeError, instead of allowing exceptions to propagate and crash callers.

Changes:

  • Wrapped feed, site, and article fetch operations (sync + async) in transport guards that return a structured error ScrapeResult on failure.
  • Refactored article scraping to separate fetching (fetch_html*) from parsing (parse_html) so parser defects still raise.
  • Added regression tests covering transport failures across feed/site/article for both sync and async APIs, plus tests ensuring parse errors still propagate.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/pyscrappy/scrapers/news.py Adds transport error handling for feed/site/article paths and introduces a helper to build consistent error results without swallowing parser errors.
tests/test_scrapers/test_news.py Adds sync/async tests asserting transport failures return structured ScrapeResult errors and that article parse failures still raise.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@vedaant00
vedaant00 marked this pull request as ready for review August 5, 2026 08:35
@vedaant00

Copy link
Copy Markdown
Collaborator

Really clean fix. I especially like that you kept parse_html outside the transport guard (and split fetch_and_parse in the article path to do it) so parser bugs still surface, plus the test asserting that. Verified the suite passes. Merging, thanks @shard872!

@vedaant00
vedaant00 merged commit eb903a9 into mldsveda:main Aug 5, 2026
6 checks passed
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.

NewsScraper crashes on a network error instead of returning a ScrapeError

3 participants