Skip to content

Commit 541c652

Browse files
thodson-usgsclaude
andcommitted
fix(nwis): forward get_record state, restore major-filter check, clarify format
Three fixes in the (deprecated) nwis module: 1. get_record's `state` argument was accepted and documented but never forwarded, so get_record(state="OH", service="site") silently ignored it and failed with a confusing "Bad Request". Forward it as the NWIS `stateCd` major filter. 2. get_dv / get_iv / get_discharge_peaks did `kwargs["sites"] = kwargs.pop("sites", sites)`, always injecting the key (as None when absent) and so defeating query_waterservices' "must specify a major filter" check — get_dv()/get_iv() with no filter raised a confusing "Bad Request" instead of the intended TypeError. Inject only when provided. 3. get_dv / get_iv passed `format="json"` explicitly alongside **kwargs, so get_dv(sites=..., format="rdb") raised "got multiple values for 'format'". These getters only parse JSON, so reject a non-json format with a clear ValueError. `wide_format` / `datetime_index` on get_record remain accepted but unimplemented (unchanged here); wiring or removing them is a separate decision for this deprecated module. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent ee653e5 commit 541c652

2 files changed

Lines changed: 57 additions & 3 deletions

File tree

dataretrieval/nwis.py

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,8 @@ def get_discharge_peaks(
276276
"""
277277
_check_sites_value_types(sites)
278278

279-
kwargs["site_no"] = kwargs.pop("site_no", sites)
279+
if sites is not None:
280+
kwargs.setdefault("site_no", sites)
280281
kwargs["begin_date"] = kwargs.pop("begin_date", start)
281282
kwargs["end_date"] = kwargs.pop("end_date", end)
282283
kwargs["multi_index"] = multi_index
@@ -534,9 +535,12 @@ def get_dv(
534535

535536
kwargs["startDT"] = kwargs.pop("startDT", start)
536537
kwargs["endDT"] = kwargs.pop("endDT", end)
537-
kwargs["sites"] = kwargs.pop("sites", sites)
538+
if sites is not None:
539+
kwargs.setdefault("sites", sites)
538540
kwargs["multi_index"] = multi_index
539541

542+
if kwargs.pop("format", "json") != "json":
543+
raise ValueError("get_dv returns JSON and does not accept a `format`.")
540544
response = query_waterservices("dv", format="json", ssl_check=ssl_check, **kwargs)
541545
df = _parse_json_or_raise(response)
542546

@@ -719,9 +723,12 @@ def get_iv(
719723

720724
kwargs["startDT"] = kwargs.pop("startDT", start)
721725
kwargs["endDT"] = kwargs.pop("endDT", end)
722-
kwargs["sites"] = kwargs.pop("sites", sites)
726+
if sites is not None:
727+
kwargs.setdefault("sites", sites)
723728
kwargs["multi_index"] = multi_index
724729

730+
if kwargs.pop("format", "json") != "json":
731+
raise ValueError("get_iv returns JSON and does not accept a `format`.")
725732
response = query_waterservices(
726733
service="iv", format="json", ssl_check=ssl_check, **kwargs
727734
)
@@ -952,6 +959,12 @@ def get_record(
952959
if service not in WATERSERVICES_SERVICES + WATERDATA_SERVICES:
953960
raise TypeError(f"Unrecognized service: {service}")
954961

962+
# Forward the documented `state` filter as the NWIS `stateCd` major filter;
963+
# it was previously accepted but silently ignored, which produced a
964+
# confusing "Bad Request" when used without `sites`.
965+
if state is not None:
966+
kwargs.setdefault("stateCd", state)
967+
955968
if service == "iv":
956969
df, _ = get_iv(
957970
sites=sites,

tests/nwis_test.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
NWIS_Metadata,
1414
_read_rdb,
1515
get_discharge_measurements,
16+
get_dv,
1617
get_gwlevels,
1718
get_info,
1819
get_iv,
@@ -120,6 +121,46 @@ def test_preformat_peaks_response():
120121
# Removed defunct gwlevels tests.
121122

122123

124+
def test_get_dv_requires_major_filter():
125+
"""Regression: get_dv() with no major filter must raise the documented
126+
TypeError. The getters injected ``kwargs["sites"] = kwargs.pop("sites",
127+
sites)``, which always set the key (None when absent) and so defeated
128+
query_waterservices' filter check, yielding a confusing Bad Request."""
129+
with warnings.catch_warnings():
130+
warnings.simplefilter("ignore")
131+
with pytest.raises(TypeError, match="major filter"):
132+
get_dv()
133+
134+
135+
def test_get_dv_rejects_non_json_format():
136+
"""Regression: get_dv passed format= explicitly alongside **kwargs, so
137+
get_dv(..., format="rdb") raised 'multiple values for format'. It now
138+
rejects a non-json format with a clear ValueError (it parses JSON)."""
139+
with warnings.catch_warnings():
140+
warnings.simplefilter("ignore")
141+
with pytest.raises(ValueError, match="JSON"):
142+
get_dv(sites="01646500", format="rdb")
143+
144+
145+
def test_get_record_forwards_state_as_statecd(monkeypatch):
146+
"""Regression: get_record's documented `state` arg was accepted but never
147+
forwarded. It now reaches the request as the NWIS `stateCd` major filter."""
148+
import dataretrieval.nwis as nwis_mod
149+
150+
captured: dict = {}
151+
152+
def fake_query_waterservices(service, format=None, ssl_check=True, **kw):
153+
captured.update(kw)
154+
raise RuntimeError("stop before network")
155+
156+
monkeypatch.setattr(nwis_mod, "query_waterservices", fake_query_waterservices)
157+
with warnings.catch_warnings():
158+
warnings.simplefilter("ignore")
159+
with pytest.raises(RuntimeError, match="stop"):
160+
get_record(state="OH", service="dv")
161+
assert captured.get("stateCd") == "OH"
162+
163+
123164
class TestDeprecationWarnings:
124165
"""Verify per-function DeprecationWarning fires with the right replacement.
125166

0 commit comments

Comments
 (0)