Skip to content

Split Water Data adapters by collection family - #351

Draft
thodson-usgs wants to merge 4 commits into
DOI-USGS:mainfrom
thodson-usgs:refactor/phase-3-adapter-structure
Draft

Split Water Data adapters by collection family#351
thodson-usgs wants to merge 4 commits into
DOI-USGS:mainfrom
thodson-usgs:refactor/phase-3-adapter-structure

Conversation

@thodson-usgs

@thodson-usgs thodson-usgs commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Dependency

This PR is stacked on and must merge after #350. Its branch starts at #350 head 966bb618; while #350 remains open, this PR intentionally includes those commits in its comparison with main.

Summary

  • split the Water Data implementation into time-series, metadata, measurements, reference, samples, and CQL collection-family modules
  • retain dataretrieval.waterdata.api as a logic-free compatibility facade with unchanged public imports, signatures, function identity, metadata, warnings, exceptions, and deprecations
  • separate OGC ambient context and schema/queryables execution from HTTP request construction
  • declare explicit exports for active service and focused Water Data modules
  • add public-contract snapshots and architecture fitness functions for facade purity, family isolation, adapter direction, and OGC boundaries
  • document the adapter facade decision and intentional service-specific return contracts

Compatibility

The contract tests freeze all 19 Water Data facade exports and exact signatures. They also verify package/facade object identity, legacy __module__, Samples private compatibility names, and the facade get_ogc_data patch target.

Validation

  • ruff check and ruff format
  • strict mypy: 50 source files clean
  • coverage run -m pytest tests/: 663 passed, 97% coverage
  • make html: succeeded (4 pre-existing warnings)
  • isolated wheel build/install: all six family modules, ogc.context, and ogc.schema importable outside the checkout; facade identity and module metadata preserved
  • pre-commit: all hooks passed

This was referenced Aug 6, 2026
Closed
Closed
Closed
@thodson-usgs
thodson-usgs force-pushed the refactor/phase-3-adapter-structure branch from c949fd1 to dd5dec9 Compare August 6, 2026 18:44
@thodson-usgs thodson-usgs mentioned this pull request Aug 6, 2026
Draft
thodson-usgs and others added 3 commits August 6, 2026 14:14
@thodson-usgs @claude
refactor(waterdata): simplify the adapter split's seams and guards
affb802 
Cleanup pass over the collection-family split. No behavior change to any
getter; the moved function bodies are untouched.

Remove a dead re-export chain. requests.py forwarded _row_cap and
_check_ogc_requests purely to preserve old private paths, and used
neither. _check_ogc_requests had no consumer at all -- engine.py imported
it only to re-export it, and every real caller already went to ogc.schema
directly. Forwarding it also gave request construction an edge to the one
OGC module that executes HTTP, so the guard asserting the opposite passed
while the rule was broken. Engine now takes the ambient row cap from its
owner, ogc.context.

Drop the __module__ rewrite loop. It mutated function objects the family
modules own, so a traceback pointed at api.py, which contains no code.
Its stated Sphinx rationale is disproved in this same package:
get_ratings and get_nearest_continuous sit in the same __all__, keep
their real __module__, and document fine.

Drop api.get_ogc_data. Nothing imported it, and it did not work as the
patch target its comment claimed -- family modules bind the name at
import, so patching the facade silently no-ops.

Derive what was frozen three times. The facade's 19 exports were written
out in api.py, architecture_test, and public_api_test; the latter two are
now derived, which also turns a change-detector into a real invariant:
the facade must export exactly the union of the family modules, so a
family gaining an export the facade forgets now fails. Likewise the
family and adapter sets are derived rather than enumerated, so a seventh
family is covered on arrival instead of being silently exempt.

Cache the two AST walks in architecture_test (240ms -> 87ms of parsing;
the file runs in 0.21s, was 0.71s), delete eight wheel-smoke assertions
that cannot fail because the imports four lines above already cover them,
and drop three decorative empty __all__ lists from internal modules.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@thodson-usgs @claude
test(architecture): check statement kinds in the facade, not just def…
0222843 
…initions

The guard certifying waterdata/api.py "logic-free" scanned its body for
def and class only. A module-level for loop lived there unremarked,
rewriting every re-exported getter's __module__ -- code owned by the
family modules, mutated from a file the test called free of
implementation. It was removed in the preceding cleanup; this is what
would have caught it.

A facade's whole legitimate vocabulary is a docstring, imports, and
assignments. Anything else is a statement that runs at import.

Verified by reintroducing the loop: the guard fires.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@thodson-usgs @claude
docs(waterdata): lead each getter with what it returns, not what the …
804d809 
…data is

Most getter docstrings opened by defining the dataset -- "Daily data provide
one data value to represent water conditions for the day" -- which tells a
reader what daily data is but not what calling the function gives them.
Sphinx summaries and IDE tooltips both show that first line alone, so the one
place a summary is guaranteed to be read was spending itself on background.

Each now opens with the call's result and keeps the domain context as the
paragraph after it. The USGS background is the valuable part of these
docstrings and is preserved; it just no longer occupies the summary slot.

get_latest_daily had get_daily's summary verbatim, so the one thing that
distinguishes it -- returning only the most recent value -- was documented
nowhere. It and get_latest_continuous now say so and point at the full-history
getter, which is the choice a reader is actually making.

get_samples loses "This is a wrapper function for the Samples database API":
callers cannot act on that. Its advice against over-filtering stays, since
that one has a consequence users feel.

The six family modules had one-line docstrings naming what they contain,
which the filename already did. Each now says what unites the family and
which neighbor to reach for instead -- discovery in metadata before
observations in time_series, cql as the escape hatch when a typed getter
cannot express the query. That navigation is the payoff of splitting the
module, and it was the part left unwritten.

Parameter sections are deliberately untouched. They are the public contract
for getters taking up to nineteen arguments, and they are reference material
a caller reads while writing the call -- not implementation detail.

Also corrects ADR 0007, which still claimed the facade preserves the legacy
__module__ value. It no longer does -- that loop was removed -- and each
function now reports the family module defining it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@thodson-usgs
thodson-usgs force-pushed the refactor/phase-3-adapter-structure branch from fb0826b to 804d809 Compare August 6, 2026 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@thodson-usgs