Skip to content

refactor: one home for option validation; retire dead engine wrappers - #372

Merged
thodson-usgs merged 1 commit into
DOI-USGS:mainfrom
thodson-usgs:refactor/generalize-shared-shapes
Aug 14, 2026
Merged

refactor: one home for option validation; retire dead engine wrappers#372
thodson-usgs merged 1 commit into
DOI-USGS:mainfrom
thodson-usgs:refactor/generalize-shared-shapes

Conversation

@thodson-usgs

Copy link
Copy Markdown
Collaborator

What

Two generalizations found by scanning the package for repeated shapes with four parallel review passes (function shapes, class/type structure, cross-cutting policy, the pagination ladder).

1. Closed-vocabulary rejection was written eleven times

Every adapter validates an argument against a Literal's get_args(), a module constant, or a mapping's keys — and each hand-wrote the raise. Eight message phrasings for one concept, so each new check was a coin flip on wording.

One had already lost that flip. get_reference_table(collection=...) told callers:

ValueError: Invalid code service: 'agency-codez'. Valid options are: (...)

There is no code_service parameter on that function. The check was copied from samples.get_codes — message and local variable name valid_code_services included — and the noun was never changed. A regression test pins the corrected wording.

dataretrieval/_validation.py::require_one_of now owns the rejection; the vocabularies stay with the adapters that define them. Migrated 11 sites: waterdata types (service, profile), samples, reference, cql, nearest, ogc.schema, wqp (dataProfile, service), nldi (find, navigation_mode).

Messages are now uniform and name the parameter the caller actually passed; the pinned assertions move with them.

Deliberately not migrated: nwis raises TypeError here rather than ValueError — changing that on an ADR 0005 quarantined module is a behavior change, not a cleanup. ratings.get_ratings reports every invalid file_type at once, which the scalar helper would lose.

2. ogc.engine._paginate was a wrapper with one production caller

It added exactly two things over transport.pagination.paginate: the OGC raise-for-status default, and preferring the running drive's client over a fresh one. The second was written twice — here and inside run_paginated.fetch (added by #371) — so it moves down into _client_for, where both callers get it and neither restates it. _walk_pages now calls paginate directly.

Also removed, each verified to have zero consumers across package, tests, docs, and notebooks:

  • _DEFAULT_DIALECT — its comment claimed tests used it; none do
  • ogc.requests._get_args — every _get_args in the repo resolves to waterdata.utils' real function of that name
  • utils._network_error

utils.USER_AGENT is deliberately left: un-underscored on a documented compatibility module, so removing it is a release decision rather than a cleanup.

Layering

_validation sits at the floor of the layers contract — it imports nothing first-party — so every layer above can reject a bad option without reaching sideways for a helper. .importlinter has exhaustive = True, so the placement is explicit by design.

Testing

  • 788 passed (one new regression test).
  • mypy --strict, ruff, xenon, complexipy, lint-imports all pass.

Considered and not done here

Four findings are real but belong in their own PRs, since they change user-visible behavior rather than structure:

  1. Empty OGC results downgrade GeoDataFrameDataFrame. _deal_with_empty builds pd.DataFrame(columns=...) unconditionally, overriding the per-page guarantee _empty_feature_frame exists to provide — 160 lines away in the same file. So get_monitoring_locations returns a GeoDataFrame when the filter matches ≥1 row and a plain DataFrame when it matches 0, breaking .geometry/.to_crs()/concat only on the empty case. Verified at runtime; one-line fix (reindex off the frame handed in).
  2. Two raise-for-status implementations disagree. ogc.errors returns a canned 403 message and never reads the body, so a revoked API_USGS_PAT is reported as "query exceeding server limits"; it also never includes the URL, so a chunked failure can't be traced to a chunk. The legacy path has the URL and the 414 remediation text but not the JSON envelope. They already share the type mapping — only the message builders diverge.
  3. OgcDialect is 5/9 of an OGC-API descriptor; base_url, output_id, and extra_id_cols travel beside it as loose keywords through nine signatures and two functools.partial sites. Bundling them is worth roughly −65 lines, but it collides with the open configuration PR (feat(config)!: resolve settings through a layered chain #353), so it should land after that merges.
  4. Deprecation advisories use four mechanisms and two warning categorieswqp's unconditional DeprecationWarning means downstream CI running -W error::DeprecationWarning cannot call any wqp getter with default arguments, while the actual NWIS qw retirement notice is a bare UserWarning that the same filter ignores.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS

@thodson-usgs
thodson-usgs force-pushed the refactor/generalize-shared-shapes branch from 6668740 to 30a667b Compare August 14, 2026 15:41
@thodson-usgs
thodson-usgs marked this pull request as ready for review August 14, 2026 15:42
@thodson-usgs
thodson-usgs merged commit ced0d1e into DOI-USGS:main Aug 14, 2026
11 checks passed
@thodson-usgs
thodson-usgs deleted the refactor/generalize-shared-shapes branch August 14, 2026 15:42
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