fix(nwis): forward get_record state, fix major-filter validation, reject bad format#310
Draft
thodson-usgs wants to merge 1 commit into
Draft
fix(nwis): forward get_record state, fix major-filter validation, reject bad format#310thodson-usgs wants to merge 1 commit into
thodson-usgs wants to merge 1 commit into
Conversation
541c652 to
0ccd573
Compare
…ect bad format
Several fixes in the deprecated `nwis` module, surfaced by the package review.
- `get_record(state=…)` was accepted and documented but never forwarded. It now
reaches the request as the NWIS `stateCd` major filter; previously it was
silently dropped, producing a confusing "Bad Request" when used without
`sites`.
- The "must specify a major filter" validation was defeated: callers injected a
filter key unconditionally (e.g. `kwargs["sites"] = kwargs.pop("sites", sites)`
set the key to None when absent), so the membership-based guards in
`query_waterservices`/`query_waterdata` always passed and a filterless request
went out as a confusing "Bad Request" instead of the intended TypeError. The
guards now require a filter that is present *and* non-None, rejecting an unset
filter at the chokepoint — no per-getter pre-filtering needed. (`utils.query`
already strips None-valued params, so this is purely a validation change.)
- `get_dv`/`get_iv`/`get_discharge_peaks`/`get_stats` each parse a fixed response
body but passed `format=` explicitly alongside `**kwargs`, so e.g.
`get_dv(sites=…, format="rdb")` raised "multiple values for 'format'". A
caller-supplied non-native `format` is now rejected with a clear ValueError via
the shared `_reject_unexpected_format` helper (json for dv/iv, rdb for
peaks/stats).
- `get_ratings` is per-site; called without one it issued a request returning an
unhelpful error page. It now fails fast with a clear TypeError (also covering
`get_record(service="ratings")`).
Adds regression tests for each. Full nwis suite passes; ruff and mypy --strict
clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
b040a74 to
f2a2503
Compare
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.
Summary
Several fixes in the (deprecated)
nwismodule, all surfaced by the package review.1.
get_record(state=…)was a dead parameter — accepted and documented, but never forwarded.get_record(state="OH", service="site")silently ignored it and, with nosites, failed with a confusing "Bad Request." Now forwarded as the NWISstateCdmajor filter.2. The "must specify a major filter" validation was defeated. The getters injected the filter key unconditionally (e.g.
kwargs["sites"] = kwargs.pop("sites", sites)setsites=Nonewhen absent), so the membership-based guards inquery_waterservices/query_waterdataalways passed and a filterless request went out as a confusing "Bad Request" instead of the intendedTypeError. Fixed once at the chokepoints: the major-filter guards now require a filter that is present and non-None. (utils.queryalready stripsNone-valued params before the request, so this is purely a validation change — no per-getter pre-filtering needed.)3.
format=collided / was silently overridden.get_dv/get_iv/get_discharge_peaks/get_statseach parse a fixed response body but passedformat=explicitly alongside**kwargs, so e.g.get_dv(sites=…, format="rdb")raisedTypeError: got multiple values for 'format'. A caller-supplied non-nativeformatis now rejected with a clearValueErrorvia a shared_reject_unexpected_formathelper (jsonfor dv/iv,rdbfor peaks/stats).4.
get_ratingsis per-site. Called without a site it issued a request that returned an unhelpful error page; it now fails fast with a clearTypeError(also coversget_record(service="ratings")).Verification (no network)
Regression tests added for each. Full
nwissuite passes;ruffandmypy --strictclean.🤖 Generated with Claude Code