Consolidate shared scraping base (update)

datasets/amfv_datasets/scraping/__init__.py

@@ -1,4 +1,8 @@
-"""Web scraping helpers and source-specific scrapers."""
+"""Web scraping helpers and source-specific scrapers.
+
+Only the shared scraping contract is re-exported here. Import a source's own
+symbols from its module, so registering a source touches one file.
+"""
 
 from amfv_datasets.scraping.base import (
     USER_AGENT,
@@ -8,7 +12,7 @@
     default_client,
     scrape_listing_documents,
 )
-from amfv_datasets.scraping.cli import OutputFormat, ScraperSource
+from amfv_datasets.scraping.cli import ALL_SOURCES, SCRAPERS, OutputFormat, Scraper
 from amfv_datasets.scraping.html import (
     LinkMode,
     absolute_unique_urls,
@@ -17,38 +21,22 @@
     first_matching_urls,
     html_to_markdown,
 )
-from amfv_datasets.scraping.nice import (
-    GuidanceListingPage,
-    GuidanceRef,
-    NiceFetchError,
-    build_guideline_text,
-    guidance_ref_from_url,
-    list_published_guidance,
-    scrape_guideline,
-    scrape_nice,
-)
 
 __all__ = [
-    "GuidanceRef",
-    "GuidanceListingPage",
+    "ALL_SOURCES",
     "LinkMode",
-    "NiceFetchError",
     "OutputFormat",
+    "SCRAPERS",
     "ScrapeError",
     "ScrapeRun",
     "ScrapedDocument",
-    "ScraperSource",
+    "Scraper",
     "USER_AGENT",
     "absolute_unique_urls",
-    "build_guideline_text",
     "clean_text",
-    "document_title",
     "default_client",
+    "document_title",
     "first_matching_urls",
-    "guidance_ref_from_url",
     "html_to_markdown",
-    "list_published_guidance",
-    "scrape_guideline",
     "scrape_listing_documents",
-    "scrape_nice",
 ]

datasets/amfv_datasets/scraping/base.py

@@ -69,12 +69,12 @@ def default_client(
     return httpx.Client(headers=client_headers, timeout=timeout, follow_redirects=follow_redirects)
 
 
-def scrape_listing_documents[ListingItemT](
+def scrape_listing_documents[ClientT, ListingItemT](
     *,
     documents: int | None,
-    client_factory: Callable[[], AbstractContextManager[httpx.Client]],
-    list_page: Callable[[httpx.Client, int], Iterable[ListingItemT]],
-    scrape_item: Callable[[httpx.Client, ListingItemT], ScrapedDocument],
+    client_factory: Callable[[], AbstractContextManager[ClientT]],
+    list_page: Callable[[ClientT, int], Iterable[ListingItemT]],
+    scrape_item: Callable[[ClientT, ListingItemT], ScrapedDocument | None],
     document_delay_seconds: float = 5.0,
     first_page_items: Iterable[ListingItemT] | None = None,
 ) -> Iterable[ScrapedDocument]:
@@ -83,9 +83,11 @@ def scrape_listing_documents[ListingItemT](
     Args:
         documents: Number of documents to scrape. When unset, listing pages are
             fetched until a page returns no items (default: None).
-        client_factory: Factory returning a context-managed HTTP client.
+        client_factory: Factory returning a context-managed client, passed to
+            `list_page` and `scrape_item` unchanged.
         list_page: Function that lists source-specific items for a page.
-        scrape_item: Function that scrapes one listed item into a document.
+        scrape_item: Function that scrapes one listed item into a document. It
+            may return None to skip a discovered item that is not a document.
         document_delay_seconds: Delay before scraping each document after the
             first one (default: 5.0).
         first_page_items: Already-fetched first listing page items. When set,
@@ -99,6 +101,7 @@ def scrape_listing_documents[ListingItemT](
     with client_factory() as client:
         page = 1
         scraped = 0
+        attempted = 0
         page_items = list(first_page_items) if first_page_items is not None else None
         while documents is None or scraped < documents:
             if page_items is None:
@@ -111,9 +114,13 @@ def scrape_listing_documents[ListingItemT](
             for item in items:
                 if documents is not None and scraped >= documents:
                     break
-                if scraped and document_delay_seconds:
+                if attempted and document_delay_seconds:
                     time.sleep(document_delay_seconds)
-                yield scrape_item(client, item)
+                document = scrape_item(client, item)
+                attempted += 1
+                if document is None:
+                    continue
+                yield document
                 scraped += 1
             page += 1
 

datasets/amfv_datasets/scraping/cli.py

@@ -8,8 +8,9 @@
 from collections.abc import Iterable
 from dataclasses import asdict
 from enum import StrEnum
+from itertools import chain
 from pathlib import Path
-from typing import Annotated, TextIO
+from typing import Annotated, Protocol, TextIO
 
 import typer
 from rich.console import Console
@@ -29,11 +30,20 @@
 from amfv_datasets.scraping.nice import scrape_nice
 
 
-class ScraperSource(StrEnum):
-    """Supported scraper sources."""
+class Scraper(Protocol):
+    """Entry point a source module exposes to run one scrape."""
 
-    ALL = "all"
-    NICE = "nice"
+    def __call__(self, *, documents: int | None, link_mode: LinkMode, url: str | None) -> ScrapeRun:
+        """Scrape a source into a run of documents."""
+        ...
+
+
+ALL_SOURCES = "all"
+
+SCRAPERS: dict[str, Scraper] = {
+    "nice": scrape_nice,
+}
+"""Scraper entry point by source name. Adding a source is an import and an entry here."""
 
 
 class OutputFormat(StrEnum):
@@ -48,7 +58,7 @@ class OutputFormat(StrEnum):
 
 
 def scrape_documents(
-    source: ScraperSource,
+    source: str,
     *,
     documents: int | None,
     link_mode: LinkMode,
@@ -57,8 +67,8 @@ def scrape_documents(
     """Configure a scrape for a source.
 
     Args:
-        source: Scraper source to run. Use `ScraperSource.ALL` to run every
-            implemented source.
+        source: Scraper source to run. Use `ALL_SOURCES` to run every registered
+            source.
         documents: Number of documents to scrape. When unset, each source runs
             until it is exhausted (default: None).
         link_mode: Whether links are kept as markdown links or stripped to their
@@ -68,13 +78,11 @@ def scrape_documents(
     if documents is not None and documents < 1:
         raise ValueError(f"documents must be at least 1; got {documents}")
 
-    for selected_source in _expand_source(source):
-        match selected_source:
-            case ScraperSource.NICE:
-                return scrape_nice(documents=documents, link_mode=link_mode, url=url)
-            case ScraperSource.ALL:
-                raise AssertionError("expanded source cannot be all")
-    raise AssertionError(f"unsupported source: {source}")
+    scrape_runs = [SCRAPERS[name](documents=documents, link_mode=link_mode, url=url) for name in _expand_source(source)]
+    if len(scrape_runs) == 1:
+        return scrape_runs[0]
+    total = None if any(run.total is None for run in scrape_runs) else sum(run.total or 0 for run in scrape_runs)
+    return ScrapeRun(documents=chain.from_iterable(run.documents for run in scrape_runs), total=total)
 
 
 def write_jsonl(documents: Iterable[ScrapedDocument], output: TextIO) -> int:
@@ -123,15 +131,17 @@ def write_markdown_files(documents: Iterable[ScrapedDocument], output_path: Path
     return count
 
 
-def _expand_source(source: ScraperSource) -> tuple[ScraperSource, ...]:
-    if source is ScraperSource.ALL:
-        return (ScraperSource.NICE,)
+def _expand_source(source: str) -> tuple[str, ...]:
+    if source == ALL_SOURCES:
+        return tuple(SCRAPERS)
+    if source not in SCRAPERS:
+        raise typer.BadParameter(f"unknown source {source!r}; choose from {', '.join([ALL_SOURCES, *SCRAPERS])}")
     return (source,)
 
 
 @app.command(help="Run a scraper and write the scraped documents.")
 def run(
-    source: Annotated[ScraperSource, typer.Option("--source", help="Scraper source to run.")],
+    source: Annotated[str, typer.Option("--source", help=f"Scraper source to run: {', '.join([ALL_SOURCES, *SCRAPERS])}.")],  # noqa: E501
     url: Annotated[str | None, typer.Option("--url", help="Source URL to scrape as a single document.")] = None,
     documents: Annotated[str, typer.Option("--documents", help="Number of documents to scrape, or 'all'.")] = "1",
     link_mode: Annotated[LinkMode, typer.Option("--links", help="Whether to keep markdown links or strip links to text.")] = LinkMode.KEEP,  # noqa: E501
@@ -168,7 +178,7 @@ def run(
         if output_path is None:
             raise typer.BadParameter("--output is required when --format markdown")
         count = write_markdown_files(scraped_documents, output_path)
-    target = url or source.value
+    target = url or source
     typer.echo(f"scraped {count} documents from {target}", err=True)
 
 
@@ -248,8 +258,10 @@ def main() -> None:
 
 
 __all__ = [
+    "ALL_SOURCES",
+    "SCRAPERS",
     "OutputFormat",
-    "ScraperSource",
+    "Scraper",
     "app",
     "LinkMode",
     "main",

datasets/amfv_datasets/scraping/html.py

@@ -118,7 +118,10 @@ def html_to_markdown(
 def _absolutize_links(html_text: str, *, base_url: str) -> str:
     root = lxml_html.fragment_fromstring(html_text, create_parent="div")
     for link in root.xpath(".//a[@href]"):
-        link.set("href", urljoin(base_url, link.get("href")))
+        href = link.get("href")
+        link.set("href", href if href.startswith("#") else urljoin(base_url, href))
+    for image in root.xpath(".//img[@src]"):
+        image.set("src", urljoin(base_url, image.get("src")))
     return "".join(lxml_html.tostring(child, encoding="unicode") for child in root)
 
 

datasets/test/test_scraping_base.py

@@ -149,3 +149,43 @@ def scrape_item(client: httpx.Client, item: str) -> ScrapedDocument:
 
     assert [document.external_id for document in documents] == ["item-1", "item-2", "item-3"]
     assert delays == [5.0, 5.0]
+
+
+def test_scrape_listing_documents_skips_non_documents_without_consuming_limit(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Skipped listing entries are delayed but do not count as returned documents."""
+    delays: list[float] = []
+
+    class _FakeClient:
+        def __enter__(self) -> "_FakeClient":
+            return self
+
+        def __exit__(self, *args: object) -> None:
+            return None
+
+    def client_factory():
+        return _FakeClient()
+
+    def list_page(client: httpx.Client, page: int) -> list[str]:
+        return ["skip", "item-1", "item-2"] if page == 1 else []
+
+    def scrape_item(client: httpx.Client, item: str) -> ScrapedDocument | None:
+        if item == "skip":
+            return None
+        return ScrapedDocument("test", item, item, f"https://example.org/{item}", "content")
+
+    monkeypatch.setattr(base.time, "sleep", delays.append)
+
+    documents = list(
+        scrape_listing_documents(
+            documents=2,
+            client_factory=client_factory,
+            list_page=list_page,
+            scrape_item=scrape_item,
+            document_delay_seconds=5.0,
+        )
+    )
+
+    assert [document.external_id for document in documents] == ["item-1", "item-2"]
+    assert delays == [5.0, 5.0]

datasets/test/test_scraping_cli.py

@@ -9,7 +9,9 @@
 
 from amfv_datasets.scraping.base import ScrapedDocument, ScrapeRun
 from amfv_datasets.scraping.cli import (
-    ScraperSource,
+    ALL_SOURCES,
+    SCRAPERS,
+    _expand_source,
     app,
     write_huggingface_dataset,
     write_jsonl,
@@ -65,13 +67,13 @@ def test_cli_run_writes_jsonl_to_stdout(monkeypatch: pytest.MonkeyPatch) -> None
     runner = CliRunner()
 
     def fake_scrape_documents(
-        source: ScraperSource,
+        source: str,
         *,
         documents: int | None,
         link_mode: LinkMode,
         url: str | None = None,
     ) -> ScrapeRun:
-        assert source is ScraperSource.NICE
+        assert source == "nice"
         assert documents == 3
         assert link_mode is LinkMode.STRIP
         assert url is None
@@ -100,7 +102,7 @@ def test_cli_run_can_disable_progress_for_file_output(monkeypatch: pytest.Monkey
     runner = CliRunner()
 
     def fake_scrape_documents(
-        source: ScraperSource,
+        source: str,
         *,
         documents: int | None,
         link_mode: LinkMode,
@@ -132,7 +134,7 @@ def test_cli_run_uses_progress_by_default(monkeypatch: pytest.MonkeyPatch, tmp_p
     progress_calls = 0
 
     def fake_scrape_documents(
-        source: ScraperSource,
+        source: str,
         *,
         documents: int | None,
         link_mode: LinkMode,
@@ -182,7 +184,7 @@ def fake_progress(
         assert total == 1
         yield from documents
 
-    monkeypatch.setattr("amfv_datasets.scraping.cli.scrape_nice", fake_scrape_nice)
+    monkeypatch.setitem(SCRAPERS, "nice", fake_scrape_nice)
     monkeypatch.setattr("amfv_datasets.scraping.cli._progress_documents", fake_progress)
 
     result = runner.invoke(
@@ -199,13 +201,13 @@ def test_cli_run_accepts_all_documents(monkeypatch: pytest.MonkeyPatch) -> None:
     runner = CliRunner()
 
     def fake_scrape_documents(
-        source: ScraperSource,
+        source: str,
         *,
         documents: int | None,
         link_mode: LinkMode,
         url: str | None = None,
     ) -> ScrapeRun:
-        assert source is ScraperSource.ALL
+        assert source == "all"
         assert documents is None
         assert link_mode is LinkMode.KEEP
         assert url is None
@@ -219,6 +221,22 @@ def fake_scrape_documents(
     assert json.loads(result.stdout.splitlines()[0])["external_id"] == "nice-ng1"
 
 
+def test_cli_run_rejects_an_unregistered_source() -> None:
+    """An unknown --source names the sources that are registered."""
+    runner = CliRunner()
+
+    result = runner.invoke(app, ["--source", "nhs"])
+
+    assert result.exit_code != 0
+    assert "'nhs'" in result.stderr
+    assert "all, nice" in result.stderr
+
+
+def test_expand_source_runs_every_registered_scraper() -> None:
+    """The all source expands to the registry rather than a hand-written list."""
+    assert _expand_source(ALL_SOURCES) == tuple(SCRAPERS)
+
+
 def _document() -> ScrapedDocument:
     return ScrapedDocument(
         source="nice",