refactor: one home for option validation; retire dead engine wrappers - #372
Merged
thodson-usgs merged 1 commit intoAug 14, 2026
Merged
Conversation
This was referenced Aug 13, 2026
thodson-usgs
force-pushed
the
refactor/generalize-shared-shapes
branch
from
August 14, 2026 15:41
6668740 to
30a667b
Compare
thodson-usgs
marked this pull request as ready for review
August 14, 2026 15:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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'sget_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:There is no
code_serviceparameter on that function. The check was copied fromsamples.get_codes— message and local variable namevalid_code_servicesincluded — and the noun was never changed. A regression test pins the corrected wording.dataretrieval/_validation.py::require_one_ofnow owns the rejection; the vocabularies stay with the adapters that define them. Migrated 11 sites: waterdatatypes(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:
nwisraisesTypeErrorhere rather thanValueError— changing that on an ADR 0005 quarantined module is a behavior change, not a cleanup.ratings.get_ratingsreports every invalidfile_typeat once, which the scalar helper would lose.2.
ogc.engine._paginatewas a wrapper with one production callerIt 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 insiderun_paginated.fetch(added by #371) — so it moves down into_client_for, where both callers get it and neither restates it._walk_pagesnow callspaginatedirectly.Also removed, each verified to have zero consumers across package, tests, docs, and notebooks:
_DEFAULT_DIALECT— its comment claimed tests used it; none doogc.requests._get_args— every_get_argsin the repo resolves towaterdata.utils' real function of that nameutils._network_errorutils.USER_AGENTis deliberately left: un-underscored on a documented compatibility module, so removing it is a release decision rather than a cleanup.Layering
_validationsits 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..importlinterhasexhaustive = True, so the placement is explicit by design.Testing
mypy --strict,ruff,xenon,complexipy,lint-importsall 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:
GeoDataFrame→DataFrame._deal_with_emptybuildspd.DataFrame(columns=...)unconditionally, overriding the per-page guarantee_empty_feature_frameexists to provide — 160 lines away in the same file. Soget_monitoring_locationsreturns aGeoDataFramewhen the filter matches ≥1 row and a plainDataFramewhen it matches 0, breaking.geometry/.to_crs()/concatonly on the empty case. Verified at runtime; one-line fix (reindexoff the frame handed in).ogc.errorsreturns a canned 403 message and never reads the body, so a revokedAPI_USGS_PATis 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.OgcDialectis 5/9 of an OGC-API descriptor;base_url,output_id, andextra_id_colstravel beside it as loose keywords through nine signatures and twofunctools.partialsites. 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.wqp's unconditionalDeprecationWarningmeans downstream CI running-W error::DeprecationWarningcannot call any wqp getter with default arguments, while the actual NWIS qw retirement notice is a bareUserWarningthat the same filter ignores.🤖 Generated with Claude Code
https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS