From 19321f96c5ad48dd63e42a1ed54511667df7791c Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 21 Aug 2026 09:13:53 -0500 Subject: [PATCH 01/10] docs: refresh contributor and agent guidance Point agents at the shared vocabulary and authoritative dependency map, replace stale file inventories with placement rules, document the current quality gates, and correct the definition of a configuration setting. --- AGENTS.md | 167 ++++++++++++++++++++++++++++++++++++++++++++--------- CONTEXT.md | 10 +++- 2 files changed, 147 insertions(+), 30 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7c821ecfd..ff8645d4c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,34 +1,147 @@ # AGENTS.md -## Scope -- Python code is in `dataretrieval/`; `dataretrieval/waterdata/` is the modern USGS Water Data API, `dataretrieval/nwis.py` is legacy/deprecated. -- `R/dataRetrieval/` is the R project copy; leave it alone unless the task asks for R work. -- Exclude `.claude/worktrees/` from searches and edits; it contains stale worktrees that pollute results. +## Start here +- **`CONTEXT.md` is the shared vocabulary** — getter, query, chunk, plan, fan-out, + page, adapter, facade, leaf, transport, collection, profile, effective + configuration, and the legacy names that are deliberately not renamed. Read it + before writing code, docstrings, or commit messages; when a term there + conflicts with a name in the code, the term wins. +- Architectural decisions and their rationale: `docs/source/architecture/decisions/` + (ADRs, referenced by number throughout the code and by `.importlinter`). +- Contributor workflow, style, and the quality gates in detail: `CONTRIBUTING.md`. -## Example Notebooks -- `demos/*.ipynb` — top-level Water Data tour: `USGS_WaterData_Introduction_Examples.ipynb` is the entry point; `_ContinuousData_`, `_DailyStatistics_`, `_DiscreteSamples_`, `_ReferenceLists_` cover individual collections; `WaterData_demo.ipynb`, `peak_streamflow_trends.ipynb`, `USGS_WaterUse_Examples.ipynb` (NWDC water-use data via `nwdc.get_wateruse`), and `R Python Vignette equivalents.ipynb` are standalone walkthroughs. -- `demos/hydroshare/*.ipynb` — per-service HydroShare examples (NLDI, NWIS WaterUse, and Water Data DailyValues / GroundwaterLevels / Measurements / ParameterCodes / Peaks / Ratings / Samples / SiteInfo / SiteInventory / Statistics / UnitValues). Mirror these when adding examples for a new collection. -- `demos/nwqn_data_pull/` — non-notebook example: a lithops/Docker batch pipeline (`retrieve_nwqn_samples.py`, `retrieve_nwqn_streamflow.py`) with its own `README.md`. -- Any `Untitled*.ipynb`, `*_test.ipynb`, or notebooks not listed here are untracked local scratch; ignore them. +## How the tree is organized +Use `ls`/`grep` for the file list; what follows is the placement logic, so you +can predict where a thing lives. + +- `dataretrieval/` — the public surface is one *adapter* module per service, + named for the service (`nldi`, `nwdc`, `ngwmn`, `streamstats`, `wqp`, and + legacy `nwis`); each owns that service's URLs, parameters, and response + quirks. `waterdata/` is the one adapter large enough to be a package, split by + collection family; its `api.py` is a compatibility facade holding no logic. + Everything else in the package is shared machinery the adapters sit on top of + — configuration, credentials, progress, exceptions, code tables, response + formats. Shared machinery below the adapter layer must not know about any + particular service. +- `dataretrieval/ogc/` — the OGC API protocol machinery (chunk planning, + filters, request building, response shaping). Shared by the two OGC services + only; `.importlinter` refuses any other importer. +- `dataretrieval/transport/` — service-neutral request machinery (HTTP, retry, + pagination, fan-out). It names no service and no protocol, and is not public API. +- Leading-underscore top-level modules are private; the dependency-free *leaves* + sit at the floor of the stack so anything may use them without pulling in the + rest of the package. Check for an existing leaf before writing a small helper. +- **`.importlinter` is the map.** Its `layers` contract lists every top-level + module in dependency order and is `exhaustive = True`, so it is both the + authoritative statement of where a module sits and the thing that fails when a + new module has no home. Read it before adding a module or an import. +- `tests/` — flat, one `*_test.py` per module or concern, organized into four + dependency-oriented layers (public contract, adapter contract, component, + cross-component) that `tests/contracts/README.md` defines and assigns files to. + `architecture_test.py` holds the fitness functions a boundary checker cannot + express (symbols, `__all__`, AST shape, cycles); pure module-to-module + direction belongs in `.importlinter` instead. +- `docs/source/` — `reference/` (one page per public module), `userguide/` + (prose topics), `architecture/` (overview + ADRs), `meta/` (project docs), + `examples/` (`.nblink` files pointing at `demos/`; the docs build executes them). +- `demos/` — one notebook per Water Data collection or topic, plus + `hydroshare/` mirroring them per service for HydroShare, and + `nwqn_data_pull/` as a non-notebook batch-pipeline example. When adding a + collection, add a demo alongside the existing ones and an `.nblink` in + `docs/source/examples/`. + +## Not part of the repo +- `R/`, `experiments/`, `build/`, `dist/`, `.kiro/`, and any `Untitled*.ipynb` or + `*_test.ipynb` at the top level are untracked local scratch — don't edit, + commit, or cite them. +- Exclude `.claude/worktrees/` from searches and edits; stale worktrees there + pollute results. ## Environment -- Use `pip install .[test,nldi]` (CI uses pip, not uv despite `uv.lock`). Docs: `pip install .[doc,nldi]`. +- `pip install .[test,nldi]` (CI uses pip, not uv, despite `uv.lock`). + Docs: `pip install .[doc,nldi]`. Gates: `pip install -e .[metrics]`. +- Python >= 3.10; the CI test matrix is 3.10, 3.13, 3.14. ## Commands -- Lint: `ruff check .` and `ruff format --check .`. -- Tests: `coverage run -m pytest tests/ && coverage report -m`, or focused like `pytest tests/waterdata_test.py::test_mock_get_samples`. -- Docs: install docs deps, `ipython kernel install --name "python3" --user`, then `make html` from `docs/`. `make docs` adds doctest+linkcheck (network-dependent). - -## Testing Gotchas -- Tests mock HTTP with `pytest-httpx`'s `httpx_mock` fixture and fixtures under `tests/data/`; keep new API tests offline. `tests/conftest.py` relaxes the fixture's strict-mode defaults (unused mocks and unmocked requests are tolerated) so rerun-on-failure works. -- `tests/nwis_test.py::test_nwis_service_live` hits live NWIS. -- `tests/waterdata_test.py` and `tests/waterdata_ratings_test.py` skip on Python <3.10, so a 3.9 run does not cover them. - -## Implementation Notes -- HTTP client is `httpx` (migrated from `requests` in #289); new code should use `httpx` and tests should mock with `httpx_mock`. -- Public download helpers return `(DataFrame, metadata)`. -- `dataretrieval/__init__.py` star-imports service modules; `dataretrieval/waterdata/__init__.py` controls Water Data exports via `__all__`. -- `dataretrieval.waterdata.utils._default_headers()` adds `X-Api-Key` from `API_USGS_PAT`; never hard-code tokens in examples or tests. -- Water Data request builders translate Python kwargs to API spellings (`skip_geometry` -> `skipGeometry`, `filter_lang` -> `filter-lang`); tests assert exact URLs/query params. -- Multi-value OGC params are comma-joined GETs, except `monitoring-locations` which POSTs CQL2 JSON. The OGC edge WAF caps total request bytes (URL + body) at ~8200, so `dataretrieval/waterdata/chunking.py` auto-splits oversized queries across sub-requests (both GET and POST paths); preserve this when adding new list-shaped kwargs. -- NLDI requires `geopandas` at import time (`pip install .[nldi]`); other modules fall back to pandas when geopandas is absent. +- Lint: `ruff check .` and `ruff format --check .` (pinned to the version in + `.pre-commit-config.yaml` and the CI lint job — keep them aligned). +- Tests: `coverage run -m pytest tests/ && coverage report -m`, or focused like + `pytest tests/waterdata_test.py::test_mock_get_samples`. +- Types: `mypy` (`strict = true` in `pyproject.toml`; CI runs it over the + PR-merged-into-main, so bare `dict`/`list` annotations fail there even if they + pass on your branch). +- Structure: `lint-imports`, `xenon --max-absolute C --max-modules B --max-average A dataretrieval`, + and `complexipy dataretrieval` (max complexity 10). All three gate merges. +- Docs: install docs deps, `ipython kernel install --name "python3" --user`, then + `make html` from `docs/`. `make docs` adds doctest+linkcheck (network-dependent). + +## Testing gotchas +- The suite is offline by default: `addopts = "-m 'not live'"`. Tests marked + `@pytest.mark.live` hit real USGS services and run on a schedule + (`.github/workflows/live-api.yml`); run them locally with `pytest tests/ -m live`. +- HTTP is mocked with `pytest-httpx`'s `httpx_mock` fixture plus fixtures under + `tests/data/`; keep new API tests offline. +- `tests/conftest.py` relaxes the fixture's strict-mode defaults and pins the + fan-out env (`API_USGS_CONCURRENT=1`, `API_USGS_RETRIES=0`, + `API_USGS_STALL_TIMEOUT=0`) plus a nonexistent `DATARETRIEVAL_CONFIG`, so tests + are deterministic and never read a developer's real config. Concurrency and + retry tests opt back in via `monkeypatch.setenv` inside the test body. + +## Error messages +Most callers here are programs — a script, a pipeline stage, an agent — so a +message is the only channel through which a caller can correct itself. Every +raise states the problem and then the move that fixes it, in that order. + +- Name the remedy, not just the fault. `"Service not recognized"` gives a caller + nothing to try next; listing the services it does accept does. For a transport + failure the remedy is whether to retry, and `transport.pagination. + paginated_failure_message()` is the model: cause, then `To recover: …`. +- Don't invent a phrasing for a check that recurs. `dataretrieval/_validation.py` + owns the wording for the four shared shapes — bad value in a closed vocabulary + (`require_one_of`), missing argument (`require_argument`), incomplete group + (`require_together`), and conflicting arguments (`require_exactly_one`, + `reject_together`). Reach for one before hand-writing a message. +- `require_argument` returns the narrowed value, so use its result rather than + re-testing for `None` to satisfy mypy — a second, unreachable message beside + the first is how the two drift apart. +- **Paste the remedy back before trusting it.** Whatever a message names must be + a real parameter of the function the *caller* called — not a private helper's + local, not a prose label — and following it literally must produce a working + call. Messages that read well have failed all three: `datetime_input` was a + private local no getter accepts, `configure(Configuration(...))` was a silent + no-op because `configure` is a context manager, `pip install + dataretrieval[nldi]` globs in zsh, and a navigation missing its `data_source` + spelled `None` into the URL and returned an empty frame. Run the corrected + call against the real service; wording review does not catch these. +- Shared checks take the caller's spelling. `_validate_data_source`, + `_format_api_dates`, and `require_one_of` all accept a `name=` so the subject + of the message is the argument that was actually passed. A helper that hard-codes + one noun reports the wrong parameter the moment a second call site reuses it. +- Prefer raising over returning something empty when the library cannot tell + "no data" from "the service misbehaved": a caller that gets an empty frame has + no signal to act on. `nldi._query_nldi` is the deliberate exception — a 200 + with a non-JSON body becomes an empty GeoDataFrame by design. + +## Implementation notes +- HTTP client is `httpx` (migrated from `requests` in #289); new code uses + `httpx` and tests mock with `httpx_mock`. +- Public getters return `(DataFrame, metadata)`. +- `dataretrieval/__init__.py` imports the service modules by name and lists them + in `__all__`; it does not star-import them, so a getter is reached through its + module (`dataretrieval.nwis.get_record`), never from the top level. `nldi` is + deliberately absent — it needs `geopandas` at import time, so it is imported on + demand. `dataretrieval/waterdata/__init__.py` controls Water Data exports via + `__all__`. +- The `API_USGS_PAT` credential is owned by the `credentials` leaf and applied as + the `X-Api-Key` header by `transport.http.default_headers()`, which sends it + only to the host it belongs to. Never hard-code tokens in examples or tests. +- Water Data request builders translate Python kwargs to API spellings + (`skip_geometry` -> `skipGeometry`, `filter_lang` -> `filter-lang`); tests + assert exact URLs and query params. +- Multi-value OGC params are comma-joined GETs, except `monitoring-locations` + which POSTs CQL2 JSON. The OGC edge WAF caps total request bytes (URL + body) + at ~8200, so `dataretrieval/ogc/chunking.py` auto-splits oversized queries + across chunks (both GET and POST paths); preserve this when adding new + list-shaped kwargs. +- NLDI requires `geopandas` at import time (`pip install .[nldi]`); other modules + fall back to pandas when geopandas is absent. diff --git a/CONTEXT.md b/CONTEXT.md index 78f87fa0a..a6bbbef69 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -128,9 +128,13 @@ it. **Configure** is the verb for applying one. **Setting** — One named tunable the caller may adjust: the API key, the concurrency cap, the retry count, the progress line, the fan-out baseline. A setting means the same thing wherever it applies, but it does not apply -everywhere: `concurrency` and `parallel_chunks` are meaningless to an adapter -that issues one request, and `ssl_check` is meaningful to only three. Which -settings an adapter accepts is part of that adapter's vocabulary. +everywhere: `concurrency` is meaningless to an adapter that issues one request +at a time, and `parallel_chunks` applies only to the two adapters whose queries +chunk. Which settings an adapter accepts is part of that adapter's vocabulary. + +A public keyword is not automatically a setting. `ssl_check` is a getter +argument on four adapters and resolves through no chain at all; the settings are +the roster the configuration system knows. **Package-wide setting** — A setting that applies to every adapter: the retry count, the progress line, the stall timeout. Set once, honored everywhere. From b4d73f699f0b9f65f3379b21b775b54a7d63c598 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 21 Aug 2026 09:13:54 -0500 Subject: [PATCH 02/10] feat(validation): make argument failures actionable Centralize the recurring missing, grouped, alternative, and conflicting argument checks. Adopt them across adapters, make every rejection name an executable remedy, prevent NLDI and nearest-observation corrections from producing silent wrong answers, and keep the associated NLDI tests offline. --- dataretrieval/_response_metadata.py | 12 +- dataretrieval/_validation.py | 215 +++++++++++++++++++++++++- dataretrieval/codes/states.py | 41 ++++- dataretrieval/credentials.py | 7 +- dataretrieval/ngwmn.py | 13 +- dataretrieval/nldi.py | 212 ++++++++++++++++++------- dataretrieval/nwdc.py | 17 +- dataretrieval/nwis.py | 74 ++++++--- dataretrieval/ogc/dates.py | 21 ++- dataretrieval/ogc/requests.py | 1 + dataretrieval/rdb.py | 9 +- dataretrieval/transport/pagination.py | 17 +- dataretrieval/waterdata/cql.py | 10 +- dataretrieval/waterdata/nearest.py | 39 ++++- dataretrieval/waterdata/ratings.py | 13 +- dataretrieval/waterdata/reference.py | 4 +- dataretrieval/waterdata/types.py | 1 + dataretrieval/wqp.py | 14 +- tests/nldi_test.py | 197 ++++++++++++++++++++++- tests/nwdc_test.py | 34 +++- tests/utils_test.py | 59 +++++++ tests/validation_test.py | 105 ++++++++++++- tests/waterdata_nearest_test.py | 51 ++++++ tests/waterdata_test.py | 29 ++++ tests/waterservices_test.py | 56 +++++-- 25 files changed, 1106 insertions(+), 145 deletions(-) diff --git a/dataretrieval/_response_metadata.py b/dataretrieval/_response_metadata.py index 68b1822e7..5c39fa807 100644 --- a/dataretrieval/_response_metadata.py +++ b/dataretrieval/_response_metadata.py @@ -53,13 +53,17 @@ def __init__(self, response: httpx.Response) -> None: # # disclaimer seems to be only part of importWaterML1 # self.disclaimer = None - # ``site_info`` is set by ``nwis`` / ``wqp``-specific metadata classes; the - # modern ``waterdata`` metadata leaves it unimplemented (use - # ``waterdata.get_monitoring_locations`` to retrieve site descriptions). + # Set by the ``nwis`` / ``wqp`` metadata subclasses only. @property def site_info(self) -> Any: raise NotImplementedError( - "site_info must be implemented by BaseMetadata children" + "This metadata object carries no site_info: only the nwis and wqp " + "metadata classes implement it, and the getter that produced this " + "result does not return site descriptions alongside data. Fetch " + "them from the same adapter -- " + "dataretrieval.waterdata.get_monitoring_locations(" + "monitoring_location_id=...) for Water Data, " + "dataretrieval.ngwmn.get_sites(...) for NGWMN." ) def __repr__(self) -> str: diff --git a/dataretrieval/_validation.py b/dataretrieval/_validation.py index 8c57d305b..3c082b436 100644 --- a/dataretrieval/_validation.py +++ b/dataretrieval/_validation.py @@ -11,11 +11,26 @@ This module owns the wording so a new check cannot invent its own. It is a leaf with no first-party imports: the vocabularies it validates against live with the adapters that define them, and only the rejection is shared. + +Three checks recur across the adapters, and each has a message shape here: a +value outside a closed vocabulary (:func:`require_one_of`), an argument that is +missing (:func:`require_argument`, :func:`require_together`), and arguments that +cannot be combined (:func:`require_exactly_one`, :func:`reject_together`). + +Every message states the problem and then the move that fixes it, in that +order. Most callers of this package are programs -- a script, a pipeline stage, +an agent -- and a program cannot infer from "Service not recognized" which +services exist. Naming the remedy is what lets the caller correct itself +without a human reading the source, so a check that cannot name one is a check +whose message is not finished. """ from __future__ import annotations -from collections.abc import Collection +from collections.abc import Collection, Mapping +from typing import TypeVar + +_T = TypeVar("_T") def _render(options: Collection[object]) -> str: @@ -69,3 +84,201 @@ def require_one_of( raise ValueError( f"Invalid {name}: {value!r}{qualifier}. Valid options are: {_render(options)}." ) + + +def _render_names(names: Collection[str], *, conjunction: str = "and") -> str: + """Format parameter *names* for a message: ``a``, ``a and b``, ``a, b and c``. + + Bare, not quoted: these are the caller's own parameter names, so they read + as identifiers to paste back into the call rather than as data values -- + which is what :func:`_render` is for. + """ + listed = list(names) + if len(listed) <= 1: + return "".join(listed) + return f"{', '.join(listed[:-1])} {conjunction} {listed[-1]}" + + +def _supplied(values: Mapping[str, object]) -> tuple[list[str], list[str]]: + """Split *values* into the names that were supplied and those that were not. + + ``None`` is the package's "not supplied" marker throughout the public + signatures, so it is the one this module tests for. A caller whose sentinel + differs -- an empty string that should count as missing -- normalizes to + ``None`` before calling, rather than this module guessing which falsy values + were meant. + """ + supplied = [name for name, value in values.items() if value is not None] + missing = [name for name, value in values.items() if value is None] + return supplied, missing + + +def require_argument( + name: str, + value: _T | None, + *, + context: str = "", + remedy: str = "", +) -> _T: + """Return *value*, or raise ``ValueError`` if it was not supplied. + + Returns the value rather than ``None`` so the check also narrows the type: + a caller that must hand an optional argument to something requiring a + concrete one writes ``x = require_argument("x", x)`` and is done. The + alternative -- validating here and re-testing for ``None`` to satisfy the + type checker -- puts a second, unreachable message next to this one, and + the two drift. + + Parameters + ---------- + name + The parameter as the caller spells it. + value + What they passed; ``None`` means not supplied. + context + When the requirement is conditional, the condition that triggered it -- + ``context="when comid is given"``. Omitted when the argument is always + required. + remedy + What to do instead, when the default ("pass a value") is not enough to + act on -- typically the accepted forms or an example value. + + Returns + ------- + The supplied value, narrowed to non-``None``. + + Raises + ------ + ValueError + If *value* is ``None``. + """ + if value is not None: + return value + when = f" {context}" if context else "" + raise ValueError(f"{name} is required{when}. {remedy or f'Pass a {name} value.'}") + + +def require_together( + values: Mapping[str, object], + *, + context: str = "", + remedy: str = "", +) -> None: + """Raise ``ValueError`` unless *values* are all supplied or all omitted. + + For arguments that only mean something as a set -- a ``lat``/``long`` pair, + a ``feature_source``/``feature_id`` pair. Passing none of them is allowed: + that is the caller declining the whole group, which is a different question + from whether the group is complete. + + Parameters + ---------- + values + Parameter name to supplied value, in the order the message should + list them. + context + Where the group applies, when more than one exists -- + ``context="for find='basin'"``. + remedy + Overrides the default remedy, which names the missing arguments to + supply and the supplied ones to drop. + + Raises + ------ + ValueError + If some but not all of *values* were supplied. + """ + supplied, missing = _supplied(values) + if not supplied or not missing: + return + where = f" {context}" if context else "" + fix = remedy or ( + f"Pass {_render_names(missing)}, or omit {_render_names(supplied)}." + ) + raise ValueError( + f"{_render_names(values)} must be given together{where}. " + f"Missing: {_render_names(missing)}. {fix}" + ) + + +def require_exactly_one( + values: Mapping[str, object], + *, + context: str = "", + remedy: str = "", +) -> None: + """Raise ``ValueError`` unless exactly one of *values* was supplied. + + For a choice between alternatives that are each sufficient on their own -- + the origin of an NLDI navigation, the location selector of an NWDC query. + Both failure directions are reported by the same check because they have + the same fix from opposite sides: supply one, or drop the rest. + + Parameters + ---------- + values + Parameter name to supplied value, in the order the message should + list them. + context + What the choice is for, when the parameter names do not say -- + ``context="as the query's location"``. + remedy + Overrides the default remedy, which is derived from which way the + check failed. + + Raises + ------ + ValueError + If none of *values* were supplied, or more than one was. + """ + supplied, _ = _supplied(values) + if len(supplied) == 1: + return + where = f" {context}" if context else "" + if supplied: + fix = remedy or f"Drop all but one of {_render_names(supplied)}." + got = _render_names(supplied) + else: + fix = remedy or f"Pass one of {_render_names(values, conjunction='or')}." + got = "none" + raise ValueError( + f"Provide exactly one of {_render_names(values, conjunction='or')}" + f"{where}. Supplied: {got}. {fix}" + ) + + +def reject_together( + values: Mapping[str, object], + *, + context: str = "", + remedy: str = "", +) -> None: + """Raise ``ValueError`` if more than one of *values* was supplied. + + The permissive sibling of :func:`require_exactly_one`: it rejects the + combination without requiring that anything be supplied at all, for + arguments that conflict but are jointly optional. + + Parameters + ---------- + values + Parameter name to supplied value, in the order the message should + list them. + context + Why they conflict, when the names do not make it evident -- + ``context="they name different origins"``. + remedy + Overrides the default remedy, which names the supplied arguments to + choose between. + + Raises + ------ + ValueError + If two or more of *values* were supplied. + """ + supplied, _ = _supplied(values) + if len(supplied) < 2: + return + why = f" -- {context}" if context else "" + fix = remedy or f"Pass only one of {_render_names(supplied, conjunction='or')}." + raise ValueError(f"{_render_names(supplied)} cannot be combined{why}. {fix}") diff --git a/dataretrieval/codes/states.py b/dataretrieval/codes/states.py index 321781b21..648c2155e 100644 --- a/dataretrieval/codes/states.py +++ b/dataretrieval/codes/states.py @@ -177,7 +177,10 @@ def _to_state_one(value: str | int, to: str) -> str: raise ValueError( f"{value!r} is not a recognized US state or the District of " f'Columbia. Provide a full name ("Wisconsin"), a two-letter postal ' - f'code ("WI"), or a two-digit ANSI/FIPS code ("55").' + f'code ("WI"), or a two-digit ANSI/FIPS code ("55"). Coverage is ' + f"the 50 states and DC only -- a US territory (Puerto Rico, Guam, " + f"US Virgin Islands, American Samoa, Northern Mariana Islands) " + f"has no entry in this table." ) return _format_state(name, to) @@ -211,11 +214,41 @@ def apply_state( native state parameters that must not be combined with ``state``; passing ``state`` alongside any of them raises ``ValueError``. Returns the (mutated) ``local_vars``. + + An unrecognized ``state`` -- a US territory, say -- is re-raised naming the + parameters in ``reject``, and only those. They are the endpoint's own state + parameters *as the caller spells them*: the mutual-exclusion guard below is + proof the getter accepts them as keyword arguments. ``into`` is deliberately + not offered, because it is a wire queryable that need not exist on the + getter's signature -- NGWMN's ``get_sites`` filters on ``state_name`` but + accepts only ``state``, so naming ``into`` there produced a remedy that + raises ``TypeError`` when followed. An endpoint with an empty ``reject`` has + no alternative spelling to offer, so it appends nothing rather than pointing + a caller back at the argument that just failed. """ state = local_vars.pop("state", None) if state is None: return local_vars - if any(local_vars.get(p) is not None for p in reject): - raise ValueError(f"Pass `state`, or {'/'.join(reject)}, but not both.") - local_vars[into] = to_state(state, to) + # Name only the parameters actually supplied: a caller told to choose + # between `state` and an argument it never passed cannot act on the message. + conflicting = " or ".join(p for p in reject if local_vars.get(p) is not None) + if conflicting: + raise ValueError( + f"state cannot be combined with {conflicting} -- they filter on " + f"the same thing. Pass state, or {conflicting}, not both." + ) + try: + local_vars[into] = to_state(state, to) + except ValueError as err: + # ``into`` leads when it is also a rejected spelling (it is the + # queryable this endpoint filters on), but it is never offered on its + # own strength -- only ``reject`` proves the getter accepts the name. + offered = dict.fromkeys(n for n in (into, *reject) if n in reject) + native = " or ".join(offered) + if not native: + raise + raise ValueError( + f"{err} Pass {native} instead -- they take the values the API " + f"itself uses, territories included." + ) from err return local_vars diff --git a/dataretrieval/credentials.py b/dataretrieval/credentials.py index a6fb2fed1..66545ba8a 100644 --- a/dataretrieval/credentials.py +++ b/dataretrieval/credentials.py @@ -132,7 +132,8 @@ def refuse_credential_keywords(names: Iterable[str]) -> None: same, and the name space belongs to the server (``get_queryables``) rather than to us. The point is to answer the caller who reasonably guesses that a credential goes here, with a ``TypeError`` naming - ``configure(Configuration(api_key=...))`` instead of a token in a URL. It + ``with configure(Configuration(api_key=...)):`` instead of a token in a + URL (the bare call is a no-op -- ``configure`` is a context manager). It errs toward rejecting for that reason. """ forbidden = set() @@ -144,7 +145,9 @@ def refuse_credential_keywords(names: Iterable[str]) -> None: spellings = ", ".join(f"{name}=" for name in sorted(forbidden)) raise TypeError( f"Credentials cannot be passed as query parameters ({spellings}); " - "use dataretrieval.configure(Configuration(api_key=...)) instead." + "wrap the call in `with dataretrieval.configure(" + "dataretrieval.Configuration(api_key=...)):`, or set the " + f"{API_KEY_ENV} environment variable." ) diff --git a/dataretrieval/ngwmn.py b/dataretrieval/ngwmn.py index 93d506963..f6ac2da4f 100644 --- a/dataretrieval/ngwmn.py +++ b/dataretrieval/ngwmn.py @@ -180,9 +180,9 @@ def get_sites( country_code, country_name : str or iterable, optional Country filters. state : str or iterable of str, optional - State/territory filter. Accepts a full name (``"Wisconsin"``), a - two-letter postal code (``"WI"``), or a two-digit ANSI/FIPS code - (``"55"``). + State filter. Accepts a full name (``"Wisconsin"``), a two-letter + postal code (``"WI"``), or a two-digit ANSI/FIPS code (``"55"``). + The 50 states and DC only; a US territory is rejected. county_name : str or iterable of str, optional County name filter. aquifer_name, site_type, aquifer_type_code : str or iterable, optional @@ -411,9 +411,10 @@ def get_providers( Parameters ---------- state : str or iterable of str, optional - State/territory filter. Accepts a full name (``"Wisconsin"``), a - two-letter postal code (``"WI"``), or a two-digit ANSI/FIPS code - (``"55"``). Only one state at a time — a multi-value state filter + State filter. Accepts a full name (``"Wisconsin"``), a two-letter + postal code (``"WI"``), or a two-digit ANSI/FIPS code (``"55"``). + The 50 states and DC only; a US territory is rejected. Only one + state at a time — a multi-value state filter returns no records for this collection. agency_code : str or iterable of str, optional Provider agency code. diff --git a/dataretrieval/nldi.py b/dataretrieval/nldi.py index 1649b3052..7f6210583 100644 --- a/dataretrieval/nldi.py +++ b/dataretrieval/nldi.py @@ -16,7 +16,13 @@ from dataretrieval import configuration as _configuration from dataretrieval._querying import _query_with_retry -from dataretrieval._validation import require_one_of +from dataretrieval._validation import ( + reject_together, + require_argument, + require_exactly_one, + require_one_of, + require_together, +) from dataretrieval.configuration import ( BaseConfiguration, _Redirectable, @@ -37,12 +43,27 @@ try: import geopandas as gpd except ImportError as err: - raise ImportError("Install geopandas to use the NLDI module.") from err + raise ImportError( + "The NLDI module requires geopandas, which is not installed. " + "Install it with `pip install 'dataretrieval[nldi]'` " + "(quoted, so the shell does not glob the brackets)." + ) from err NLDI_API_BASE_URL = "https://api.water.usgs.gov/nldi/linked-data" _AVAILABLE_DATA_SOURCES = None _CRS = "EPSG:4326" _VALID_NAVIGATION_MODES = ("UM", "DM", "UT", "DD") +#: The modes rendered for a message. Built from the tuple above rather than +#: written beside it, so a mode added there cannot go unmentioned here. +_NAVIGATION_MODES_HINT = ( + f"Pass one of {', '.join(repr(mode) for mode in _VALID_NAVIGATION_MODES)}." +) +#: The two ways to name an origin. Shared by the conflict check and the +#: nothing-supplied check so the same pair of ways forward is offered either way. +_ORIGIN_HINT = ( + "Navigate from a comid, e.g. comid=13294314, or from a " + "feature_source/feature_id pair -- not both" +) def _api_base() -> str: @@ -156,7 +177,7 @@ def get_flowlines( navigation_mode = _validate_navigation_mode(navigation_mode) _validate_feature_source_comid(feature_source, feature_id, comid) if feature_source: - _validate_data_source(feature_source) + _validate_data_source(feature_source, name="feature source") url, query_params = _navigation_request( feature_source=feature_source, feature_id=feature_id, @@ -208,9 +229,15 @@ def get_basin( ... ) """ # validate the feature source - _validate_data_source(feature_source) - if not feature_id: - raise ValueError("feature_id is required") + _validate_data_source(feature_source, name="feature source") + require_argument( + "feature_id", + feature_id or None, + context=f"to say which {feature_source} feature the basin drains to", + remedy=( + "Pass the id as its source spells it, e.g. feature_id='USGS-01031500'." + ), + ) url = f"{_api_base()}/{feature_source}/{feature_id}/basin" simplified_str = str(simplified).lower() @@ -331,16 +358,25 @@ def _validate_lat_long_origin( feature_source: str | None, feature_id: str | None, ) -> None: - """Raise if lat/long is combined with another origin type.""" - if comid is not None: - raise ValueError( - "Provide only one origin type - comid cannot be provided with lat or long" - ) - if feature_source is not None or feature_id is not None: - raise ValueError( - "Provide only one origin type - feature_source and feature_id cannot" - " be provided with lat or long" - ) + """Raise if lat/long is combined with another origin type. + + Called with a lat/long already supplied, so the pair is passed as a + present marker: the conflict is between origin *types*, and naming the + type is what tells the caller which argument to drop. + """ + reject_together( + { + "lat/long": True, + "comid": comid, + "feature_source": feature_source, + "feature_id": feature_id, + }, + context="each names a different origin to navigate from", + remedy=( + "Navigate from a point (lat and long), a comid, or a " + "feature_source/feature_id pair -- one origin per call." + ), + ) def _get_features_request( @@ -356,35 +392,57 @@ def _get_features_request( stop_comid: int | None, ) -> tuple[str, dict[str, str]]: """Validate a feature origin and build its NLDI request parameters.""" - if (lat is None) != (long is None): - raise ValueError("Both lat and long are required") + require_together( + {"lat": lat, "long": long}, + context="to navigate from a point", + remedy="Pass both, e.g. lat=43.087, long=-89.509.", + ) if lat is not None: _validate_lat_long_origin(comid, feature_source, feature_id) return f"{_api_base()}/comid/position", {"coords": f"POINT({long} {lat})"} - if (comid is not None or data_source is not None) and navigation_mode is None: - raise ValueError( - "navigation_mode is required if comid or data_source is provided" + if comid is not None or data_source is not None: + require_argument( + "navigation_mode", + navigation_mode, + context="when comid or data_source is given", + remedy=_NAVIGATION_MODES_HINT, ) _validate_feature_source_comid(feature_source, feature_id, comid) if data_source is not None: _validate_data_source(data_source) if feature_source is not None: - _validate_data_source(feature_source) + _validate_data_source(feature_source, name="feature source") if not navigation_mode: return f"{_api_base()}/{feature_source}/{feature_id}", {} + # Before the data_source check below: a caller who mistyped the mode should + # hear about the mode, not be sent to fix a second argument first. navigation_mode = _validate_navigation_mode(navigation_mode) + # The navigation's tail is the data source, so a missing one is spelled + # "None" into the path and the service answers 200 with zero features. + data_source = require_argument( + "data_source", + data_source, + context=( + "when navigation_mode is given -- it names which features to " + "return along the navigation" + ), + remedy=( + "Pass the source of the features, e.g. data_source='nwissite'. " + "For the flowlines themselves call get_flowlines() instead." + ), + ) url, query_params = _navigation_request( feature_source=feature_source, feature_id=feature_id, comid=comid, navigation_mode=navigation_mode, distance=distance, - tail=f"{data_source}", + tail=data_source, ) if stop_comid is not None: query_params["stopComid"] = str(stop_comid) @@ -428,9 +486,27 @@ def get_features_by_data_source(data_source: str) -> gpd.GeoDataFrame: def _search_basin(feature_source: str | None, feature_id: str | None) -> dict[str, Any]: """Handle ``find='basin'`` for :func:`search`.""" - if feature_source is None or feature_id is None: - raise ValueError("feature_source and feature_id are required to find a basin") - return get_basin(feature_source=feature_source, feature_id=feature_id, as_json=True) + remedy = ( + "Pass both, e.g. feature_source='WQP', feature_id='USGS-01031500'; " + "a basin has no other origin." + ) + # ``require_together`` reports a half-supplied pair, naming both sides at + # once; it permits the pair being absent entirely, which the two checks + # below reject. Together they cover every way the origin can be incomplete. + require_together( + {"feature_source": feature_source, "feature_id": feature_id}, + context="for find='basin'", + remedy=remedy, + ) + return get_basin( + feature_source=require_argument( + "feature_source", feature_source, context="for find='basin'", remedy=remedy + ), + feature_id=require_argument( + "feature_id", feature_id, context="for find='basin'", remedy=remedy + ), + as_json=True, + ) def _search_flowlines( @@ -442,11 +518,12 @@ def _search_flowlines( comid: int | None, ) -> dict[str, Any]: """Handle ``find='flowlines'`` for :func:`search`.""" - if navigation_mode is None: - raise ValueError( - "navigation_mode is required for find='flowlines';" - f" allowed values are {_VALID_NAVIGATION_MODES}" - ) + navigation_mode = require_argument( + "navigation_mode", + navigation_mode, + context="for find='flowlines'", + remedy=_NAVIGATION_MODES_HINT, + ) return get_flowlines( navigation_mode=navigation_mode, distance=distance, @@ -535,19 +612,28 @@ def search( ... ) """ - if (lat is None) != (long is None): - raise ValueError("Both lat and long are required") + require_together( + {"lat": lat, "long": long}, + context="to search from a point", + remedy="Pass both, e.g. lat=43.087, long=-89.509.", + ) find = cast("Literal['basin', 'flowlines', 'features']", find.lower()) require_one_of(find, ("basin", "flowlines", "features"), name="find") if lat is not None and find != "features": raise ValueError( - f"Invalid value for find: {find} - lat/long is to get features not {find}" + f"find={find!r} cannot be combined with lat/long -- a point origin " + "resolves to features only. Pass find='features' to keep the " + "point origin, or drop lat and long and pass the origin " + f"{find} takes: feature_source and feature_id" + f"{' or comid' if find == 'flowlines' else ''}." ) if comid is not None and find == "basin": raise ValueError( - "Invalid value for find: basin - comid is to get features" - " or flowlines not basin" + "find='basin' cannot be combined with comid -- a basin is looked " + "up by feature, not by flowline. Pass feature_source and " + "feature_id instead, or keep comid and pass find='flowlines' " + "or find='features'." ) if lat is not None: @@ -577,7 +663,7 @@ def search( ) -def _validate_data_source(data_source: str) -> None: +def _validate_data_source(data_source: str, *, name: str = "data source") -> None: # A helper function to validate user specified data source/feature source global _AVAILABLE_DATA_SOURCES @@ -592,23 +678,28 @@ def _validate_data_source(data_source: str) -> None: raise ValueError( "NLDI data-source catalog returned an unexpected shape; " "expected a list of {'source': ..., ...} objects, got: " - f"{available_data_sources!r}" + f"{available_data_sources!r}. If you set " + "NldiConfiguration(base_url=...), point it at the linked-data " + "root, e.g. base_url='https://api.water.usgs.gov/nldi/" + "linked-data'; otherwise the service returned an unexpected " + "body -- retry later." ) _AVAILABLE_DATA_SOURCES = [ds["source"] for ds in available_data_sources] if data_source not in _AVAILABLE_DATA_SOURCES: err_msg = ( - f"Invalid data source '{data_source}'." - f" Available data sources are: {_AVAILABLE_DATA_SOURCES}" + f"Invalid {name} '{data_source}'." + f" Available sources are: {_AVAILABLE_DATA_SOURCES}" ) raise ValueError(err_msg) def _validate_navigation_mode(navigation_mode: str | None) -> str: - if navigation_mode is None: - raise ValueError( - f"navigation_mode is required; allowed values are {_VALID_NAVIGATION_MODES}" - ) + navigation_mode = require_argument( + "navigation_mode", + navigation_mode, + remedy=_NAVIGATION_MODES_HINT, + ) normalized = navigation_mode.upper() require_one_of(normalized, _VALID_NAVIGATION_MODES, name="navigation_mode") return normalized @@ -617,19 +708,28 @@ def _validate_navigation_mode(navigation_mode: str | None) -> str: def _validate_feature_source_comid( feature_source: str | None, feature_id: str | None, comid: int | None ) -> None: - if feature_source is not None and feature_id is None: - raise ValueError("feature_id is required if feature_source is provided") - if feature_id is not None and feature_source is None: - raise ValueError("feature_source is required if feature_id is provided") - if comid is not None and feature_source is not None: - raise ValueError( - "Specify only one origin type - comid and feature_source" - " cannot be provided together" - ) - if comid is None and feature_source is None: - raise ValueError( - "Specify one origin type - comid or feature_source is required" + if comid is not None: + # Half a feature pair beside a comid is a conflict, not a gap: advising + # the caller to complete the pair would only raise the conflict next. + reject_together( + { + "comid": comid, + "feature_source": feature_source, + "feature_id": feature_id, + }, + context="they name different origins", + remedy=f"{_ORIGIN_HINT}.", ) + require_together( + {"feature_source": feature_source, "feature_id": feature_id}, + context="to name one feature between them", + remedy=("Pass both, e.g. feature_source='WQP', feature_id='USGS-01031500'."), + ) + require_exactly_one( + {"comid": comid, "feature_source": feature_source}, + context="as the origin to navigate from", + remedy=f"{_ORIGIN_HINT}, and not neither.", + ) @dataclass(frozen=True) diff --git a/dataretrieval/nwdc.py b/dataretrieval/nwdc.py index 667053f1b..60848972f 100644 --- a/dataretrieval/nwdc.py +++ b/dataretrieval/nwdc.py @@ -314,7 +314,7 @@ def _resolve_locations( locations = _LOCATION_BUILDERS[name](value) if not locations: raise ValueError( - "The chosen location selector is empty; pass at least one value." + f"{name} was given an empty value. Pass at least one {name} value." ) return locations @@ -413,7 +413,10 @@ def _read_csv_page(response: httpx.Response) -> pd.DataFrame: # zeros, never an empty body — but keep the typed-error contract if it # ever returns one rather than leaking a bare pandas exception. raise DataRetrievalError( - f"NWDC returned an empty response body (URL: {response.url})." + f"NWDC returned an empty response body (URL: {response.url}). " + "The service signals 'no data' with a 400 or with zero-valued " + "rows, so an empty body is unexpected: retry once, and report it " + "if it persists." ) from exc @@ -464,7 +467,15 @@ def _nwdc_error_detail(response: httpx.Response) -> str | None: body = response.json() except ValueError: return None - return body.get("detail") if isinstance(body, dict) else None + detail = body.get("detail") if isinstance(body, dict) else None + if not isinstance(detail, str): + # A validation envelope spells ``detail`` as a list of error objects; + # only prose belongs in a message. + return None + if detail.startswith("Invalid model name"): + # The service names the rejected value but not the accepted ones. + return f"{detail.rstrip('.')}. Valid models are: {', '.join(MODELS)}." + return detail @dataclass(frozen=True) diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 9221513ec..b1fdac39a 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -121,8 +121,10 @@ def _parse_json_or_raise(response: httpx.Response) -> pd.DataFrame: ): raise ValueError( f"Received HTML response instead of JSON from {response.url} " - f"(Status: {response.status_code}). This often indicates " - "that the service is currently unavailable." + f"(Status: {response.status_code}). This usually means the " + "service is down or rate-limiting. Wait and retry; if it " + "persists, check https://waterservices.usgs.gov/ or switch to " + "the dataretrieval.waterdata getters." ) from e raise @@ -390,15 +392,30 @@ def query_waterdata( ] if not any(key in kwargs for key in major_params + bbox_params): - raise TypeError("Query must specify a major filter: site_no, stateCd, bBox") + raise TypeError( + "Query must specify a major filter. Pass one of " + f"{', '.join(major_params)}, or all four bounding-box corners " + f"({', '.join(bbox_params)}) together with " + "coordinate_format='decimal_degrees'." + ) elif any(key in kwargs for key in bbox_params) and not all( key in kwargs for key in bbox_params ): - raise TypeError("One or more lat/long coordinates missing or invalid.") + absent = [key for key in bbox_params if key not in kwargs] + raise TypeError( + "A bounding box needs all four corners. Missing: " + f"{', '.join(absent)}. Pass them along with " + "coordinate_format='decimal_degrees', or drop the bounding box " + f"and filter with {' or '.join(major_params)} instead." + ) - if service not in WATERDATA_SERVICES: - raise TypeError("Service not recognized") + if service != "peaks": + raise TypeError( + f"Unrecognized service: {service!r}. query_waterdata serves " + "'peaks'. For rating tables call nwis.get_ratings(site=...), " + "which is served from a different endpoint." + ) url = WATERDATA_URL + service @@ -447,15 +464,18 @@ def query_waterservices( The response object from the API request to the web service. """ - if not any( - key in kwargs for key in ["sites", "stateCd", "bBox", "huc", "countyCd"] - ): + major_filters = ["sites", "stateCd", "bBox", "huc", "countyCd"] + if not any(key in kwargs for key in major_filters): raise TypeError( - "Query must specify a major filter: sites, stateCd, bBox, huc, or countyCd" + "Query must specify a major filter. Pass one of " + f"{', '.join(major_filters)}." ) if service not in WATERSERVICES_SERVICES: - raise TypeError("Service not recognized") + raise TypeError( + f"Unrecognized service: {service!r}. query_waterservices serves " + f"{', '.join(repr(name) for name in WATERSERVICES_SERVICES)}." + ) if "format" not in kwargs: kwargs["format"] = "rdb" @@ -734,8 +754,8 @@ def get_iv( def get_pmcodes(**kwargs: Any) -> NoReturn: """Defunct: use ``waterdata.get_reference_table(collection='parameter-codes')``.""" raise NameError( - "`nwis.get_pmcodes` has been replaced " - "with `get_reference_table(collection='parameter-codes')`." + "`nwis.get_pmcodes` has been replaced with " + "`waterdata.get_reference_table(collection='parameter-codes')`." ) @@ -895,7 +915,7 @@ def get_record( - 'gwlevels': (defunct) use `waterdata.get_continuous`, `waterdata.get_daily`, or `waterdata.get_field_measurements` - 'pmcodes': (defunct) use `waterdata.get_reference_table` - - 'water_use': (defunct) no replacement available + - 'water_use': (defunct) use `nwdc.get_wateruse` - 'ratings': get rating table - 'stat': get statistics ssl_check: bool, optional @@ -946,7 +966,7 @@ def get_record( "(discrete)" ), "pmcodes": "`waterdata.get_reference_table`", - "water_use": "no replacement available", + "water_use": "`nwdc.get_wateruse`", } if service in defunct_replacements: raise NameError( @@ -954,8 +974,14 @@ def get_record( f"get_record. Use {defunct_replacements[service]} instead." ) - if service not in WATERSERVICES_SERVICES + WATERDATA_SERVICES: - raise TypeError(f"Unrecognized service: {service}") + supported = WATERSERVICES_SERVICES + WATERDATA_SERVICES + if service not in supported: + raise TypeError( + f"Unrecognized service: {service!r}. get_record serves " + f"{', '.join(repr(name) for name in supported)}. New work should " + "use the dataretrieval.waterdata getters instead; NWIS is " + "deprecated." + ) if service == "iv": df, _ = get_iv( @@ -1005,8 +1031,12 @@ def get_record( df, _ = get_stats(sites=sites, ssl_check=ssl_check, **kwargs) return df - else: - raise TypeError(f"{service} service not yet implemented") + else: # pragma: no cover - a recognized service with no branch above + raise TypeError( + f"The {service!r} service is recognized but get_record has no " + "handler for it. This is a bug in dataretrieval; please report it " + "at https://github.com/DOI-USGS/dataretrieval-python/issues." + ) def _site_block_boundaries(site_list: list[str]) -> list[int]: @@ -1124,7 +1154,11 @@ def _read_rdb(rdb: str) -> pd.DataFrame: def _check_sites_value_types(sites: list[str] | str | None) -> None: if sites and not isinstance(sites, list) and not isinstance(sites, str): - raise TypeError("sites must be a string or a list of strings") + raise TypeError( + "sites must be a site number as a string, or a list of them, not " + f"{type(sites).__name__}. Pass sites='01491000' for one site, or " + "sites=['01491000', '01645000'] for several." + ) class NWIS_Metadata(BaseMetadata): diff --git a/dataretrieval/ogc/dates.py b/dataretrieval/ogc/dates.py index f12ced9cf..1a5a4e095 100644 --- a/dataretrieval/ogc/dates.py +++ b/dataretrieval/ogc/dates.py @@ -83,13 +83,14 @@ def _format_one(dt: str | None, *, date: bool) -> str | None: def _coerce_to_list( datetime_input: str | Sequence[str | None], + name: str = "date input", ) -> list[str | None]: """Normalize datetime input to a list, raising on invalid shapes.""" if isinstance(datetime_input, str): return [datetime_input] if isinstance(datetime_input, Mapping): raise TypeError( - f"date input must be a string or sequence of strings, " + f"{name} must be a string or sequence of strings, " f"not {type(datetime_input).__name__}." ) return list(datetime_input) @@ -106,7 +107,11 @@ def _all_blank(items: list[str | None]) -> bool: def _format_api_dates( - datetime_input: str | Sequence[str | None] | None, date: bool = False + datetime_input: str | Sequence[str | None] | None, + date: bool = False, + *, + name: str = "date input", + allow_duration: bool = True, ) -> str | None: """ Formats date or datetime input(s) for use with an API. @@ -157,13 +162,21 @@ def _format_api_dates( if datetime_input is None: return None - items = _coerce_to_list(datetime_input) + items = _coerce_to_list(datetime_input, name) if _all_blank(items): return None if len(items) > 2: - raise ValueError("datetime_input should only include 1-2 values") + raise ValueError( + f"{name} takes at most 2 values, got {len(items)}: {items!r}. " + + ( + "Pass one value for an instant or a duration ('2020-01-01', 'P7D'), " + if allow_duration + else "Pass one value for an instant ('2020-01-01'), " + ) + + "or two for a closed interval ('2020-01-01', '2020-12-31')." + ) # Pass through duration ("P7D", "PT36H") and pre-formatted interval ("a/b") if len(items) == 1 and isinstance(items[0], str) and _is_passthrough(items[0]): diff --git a/dataretrieval/ogc/requests.py b/dataretrieval/ogc/requests.py index c82752cf5..f5ed26fe0 100644 --- a/dataretrieval/ogc/requests.py +++ b/dataretrieval/ogc/requests.py @@ -179,6 +179,7 @@ def _construct_api_requests( if key in kwargs: kwargs[key] = _format_api_dates( kwargs[key], + name=key, date=( collection in dialect.date_only_services and key != "last_modified" ), diff --git a/dataretrieval/rdb.py b/dataretrieval/rdb.py index 2a5a6c24b..989b0aae7 100644 --- a/dataretrieval/rdb.py +++ b/dataretrieval/rdb.py @@ -51,8 +51,13 @@ def read_rdb(text: str, dtypes: dict[str, type] | None = None) -> pd.DataFrame: """ if "" in text.lower() or "" in text.lower(): raise ValueError( - "Received HTML response instead of RDB. This often indicates " - "that the service has been moved or is currently unavailable." + "Received an HTML response instead of RDB, which usually means " + "the service is degraded, has moved, or returned an error page " + "rather than that the query was wrong. Retry once after a short " + "wait; if the same HTML comes back, the endpoint has most likely " + "been retired -- see " + "https://waterdata.usgs.gov/blog/api-waterservices-decom/ and " + "migrate to the dataretrieval.waterdata getters." ) lines = text.splitlines() diff --git a/dataretrieval/transport/pagination.py b/dataretrieval/transport/pagination.py index 1cc2e27cb..721c81b49 100644 --- a/dataretrieval/transport/pagination.py +++ b/dataretrieval/transport/pagination.py @@ -21,6 +21,7 @@ _merge_response, _safe_elapsed, ) +from dataretrieval.credentials import accepts_api_key from dataretrieval.exceptions import DataRetrievalError, RateLimited # One-way: ``fanout`` does not import this module, so this edge cannot cycle. @@ -56,7 +57,11 @@ async def _client_for( yield new -def paginated_failure_message(pages_collected: int, cause: BaseException) -> str: +def paginated_failure_message( + pages_collected: int, + cause: BaseException, + url: str | httpx.URL | None = None, +) -> str: """Build a recovery-oriented message for an interrupted page walk.""" cause_str = str(cause).removesuffix(".") if not cause_str.strip(): @@ -65,11 +70,13 @@ def paginated_failure_message(pages_collected: int, cause: BaseException) -> str action = "wait for the rate-limit window to reset and retry" else: action = "retry the request (possibly after a short backoff)" + # "get a token" is only actionable against the host that honours one. + token_advice = ", or obtain an API token" if accepts_api_key(url) else "" return ( f"Paginated request failed after collecting {pages_collected} " f"page(s): {cause_str}. To recover: {action}, reduce the " f"request size (e.g. fewer locations, a shorter time range, or " - f"a smaller ``limit``), or obtain an API token." + f"a smaller ``limit``){token_advice}." ) @@ -111,7 +118,9 @@ def report_page(page: httpx.Response, frame: pd.DataFrame) -> None: frame, cursor = parse_response(response) except Exception as exc: # noqa: BLE001 logger.warning("Initial response parse failed.") - raise DataRetrievalError(paginated_failure_message(0, exc)) from exc + raise DataRetrievalError( + paginated_failure_message(0, exc, response.url) + ) from exc frames = [frame] nrows = len(frame) @@ -137,7 +146,7 @@ def report_page(page: httpx.Response, frame: pd.DataFrame) -> None: "Request failed at cursor %r. Data download interrupted.", cursor ) raise DataRetrievalError( - paginated_failure_message(len(frames), exc) + paginated_failure_message(len(frames), exc, response.url) ) from exc final_response = _merge_response( diff --git a/dataretrieval/waterdata/cql.py b/dataretrieval/waterdata/cql.py index f4579bea1..2cc740c60 100644 --- a/dataretrieval/waterdata/cql.py +++ b/dataretrieval/waterdata/cql.py @@ -15,7 +15,6 @@ import pandas as pd from dataretrieval._deprecation import REMOVALS -from dataretrieval._validation import require_one_of from dataretrieval.waterdata.utils import ( _OUTPUT_ID_BY_COLLECTION, _accept_legacy_kwargs, @@ -138,7 +137,14 @@ def get_cql( ... ' "02070010%"]}', ... ) """ - require_one_of(collection, sorted(_OUTPUT_ID_BY_COLLECTION), name="collection") + if collection not in _OUTPUT_ID_BY_COLLECTION: + raise ValueError( + f"Invalid collection: {collection!r}. get_cql supports: " + f"{', '.join(repr(c) for c in sorted(_OUTPUT_ID_BY_COLLECTION))}. " + "The service serves more collections than these; for the full " + "inventory call get_queryables(), which accepts " + "any collection string." + ) # ``dict`` is the pythonic input — serialize on the way out. ``str`` is sent # verbatim so callers who already have a CQL2 doc (e.g. imported from a diff --git a/dataretrieval/waterdata/nearest.py b/dataretrieval/waterdata/nearest.py index 5d030923f..650162cd7 100644 --- a/dataretrieval/waterdata/nearest.py +++ b/dataretrieval/waterdata/nearest.py @@ -162,7 +162,11 @@ def get_nearest_continuous( Additional keyword arguments forwarded to ``get_continuous`` (e.g. ``statistic_id``, ``approval_status``, ``properties``). Passing ``time``, ``filter``, or ``filter_lang`` raises - ``TypeError`` — this function builds those itself. + ``TypeError`` — this function builds those itself. A ``properties`` + list gains ``time`` and ``monitoring_location_id`` if it omits + them: the match is computed against the first and grouped by the + second, so the returned frame carries both columns even when they + were not requested. Returns ------- @@ -233,10 +237,15 @@ def get_nearest_continuous( window_td = pd.Timedelta(window) if len(target_index) == 0: - raise ValueError("targets must contain at least one timestamp") + raise ValueError( + "targets is empty; there is nothing to find a nearest value for. " + "Pass at least one timestamp, e.g. targets=['2024-01-01 12:00'] " + "or a pandas DatetimeIndex." + ) selector = _NearestSelector(target_index, window_td, on_tie) filter_expr = _build_window_or_filter(target_index, window_td) + kwargs = _with_required_properties(kwargs) try: df, md = get_continuous( monitoring_location_id=monitoring_location_id, @@ -251,6 +260,25 @@ def get_nearest_continuous( return selector.select(df), md +def _with_required_properties(kwargs: dict[str, Any]) -> dict[str, Any]: + """Keep the columns the nearest-match needs in a caller's ``properties``. + + ``time`` is what the match is computed against and + ``monitoring_location_id`` is what it groups by, so a ``properties`` list + omitting either silently collapses every site into one row per target + rather than failing. Added rather than rejected: the caller asked for + columns, not for a lecture about which ones this getter needs. + """ + properties = kwargs.get("properties") + if properties is None: + return kwargs + names = [properties] if isinstance(properties, str) else list(properties) + missing = [c for c in ("time", "monitoring_location_id") if c not in names] + if not missing: + return kwargs + return {**kwargs, "properties": names + missing} + + def _select_nearest_rows( df: pd.DataFrame, targets: pd.DatetimeIndex, @@ -260,8 +288,11 @@ def _select_nearest_rows( """Apply the public nearest-per-target shape to continuous rows.""" if "time" not in df.columns: raise ValueError( - "get_nearest_continuous requires a 'time' column in the response; " - "if a `properties` kwarg was passed, include 'time' in it" + "get_nearest_continuous requires a 'time' column in the " + "response; if a `properties` kwarg was passed, include 'time' in " + "it -- and 'monitoring_location_id' when more than one site is " + "requested, or observations from different sites collapse into " + "one row per target." ) if df.empty: return _empty_nearest_result(df) diff --git a/dataretrieval/waterdata/ratings.py b/dataretrieval/waterdata/ratings.py index b327bbdf0..c64c50a3a 100644 --- a/dataretrieval/waterdata/ratings.py +++ b/dataretrieval/waterdata/ratings.py @@ -176,7 +176,11 @@ def get_ratings( file_types = _as_list(file_type) _validate_file_types(file_types) _validate_time_no_duration(time) - time_str = _format_api_dates(time) if time is not None else None + time_str = ( + _format_api_dates(time, name="time", allow_duration=False) + if time is not None + else None + ) # Mirror R: pin file_type server-side only when one type is requested. server_file_type = file_types[0] if len(file_types) == 1 else None @@ -346,7 +350,12 @@ async def _fetch_rating( fid = feature["id"] href = _asset_href(feature) if not href: - raise ValueError(f"STAC feature {fid!r} carries no data asset href.") + raise ValueError( + f"STAC feature {fid!r} carries no data asset href, so its rating " + "cannot be downloaded. Retrying will not help; exclude this " + "monitoring location, or report it if the rating is expected to " + "exist." + ) headers = _default_headers(href) session = active_client() if session is None: diff --git a/dataretrieval/waterdata/reference.py b/dataretrieval/waterdata/reference.py index 38ab0c95d..b5ab18fdb 100644 --- a/dataretrieval/waterdata/reference.py +++ b/dataretrieval/waterdata/reference.py @@ -95,8 +95,8 @@ def get_reference_table( require_one_of(collection, get_args(METADATA_COLLECTIONS), name="collection") # Give the ID column the collection name, singularized and underscored. - if collection == "counties": - output_id = "county" + if collection in ("counties", "countries"): + output_id = collection[:-3] + "y" # county / country elif collection.endswith("s"): output_id = collection[:-1].replace("-", "_") else: diff --git a/dataretrieval/waterdata/types.py b/dataretrieval/waterdata/types.py index bdc408e0d..c234513df 100644 --- a/dataretrieval/waterdata/types.py +++ b/dataretrieval/waterdata/types.py @@ -33,6 +33,7 @@ "coordinate-datum-codes", "coordinate-method-codes", "counties", + "countries", "hydrologic-unit-codes", "medium-codes", "national-aquifer-codes", diff --git a/dataretrieval/wqp.py b/dataretrieval/wqp.py index 605fe44fd..8b9fbb8dc 100644 --- a/dataretrieval/wqp.py +++ b/dataretrieval/wqp.py @@ -744,9 +744,19 @@ def _check_kwargs(kwargs: dict[str, Any]) -> dict[str, Any]: mimetype = kwargs.get("mimeType") if mimetype == "geojson": - raise NotImplementedError("GeoJSON not yet supported. Set 'mimeType=csv'.") + raise NotImplementedError( + "GeoJSON is not supported by this package. Pass mimeType='csv' " + "(the default); coordinates are not in the default Result " + "profile -- get them from dataretrieval.wqp.what_sites " + "(LatitudeMeasure/LongitudeMeasure), from the returned " + "metadata's .site_info, or by passing legacy=False, whose " + "WQX3.0 profiles include Location_Latitude/Location_Longitude." + ) elif mimetype != "csv" and mimetype is not None: - raise ValueError("Invalid mimeType. Set 'mimeType=csv'.") + raise ValueError( + f"Invalid mimeType: {mimetype!r}. Pass mimeType='csv', or omit it " + "-- csv is the only format this package parses." + ) else: kwargs["mimeType"] = "csv" diff --git a/tests/nldi_test.py b/tests/nldi_test.py index 8b8f4af04..948f82de1 100644 --- a/tests/nldi_test.py +++ b/tests/nldi_test.py @@ -7,9 +7,11 @@ import dataretrieval.nldi as nldi from dataretrieval.nldi import ( NLDI_API_BASE_URL, + _validate_feature_source_comid, _validate_navigation_mode, get_basin, get_features, + get_features_by_data_source, get_flowlines, search, ) @@ -188,24 +190,41 @@ def test_get_features_by_lat_long(httpx_mock): @pytest.mark.parametrize( - ("kwargs", "message"), + ("kwargs", "problem", "remedy"), [ - ({"lat": 43.087}, "Both lat and long are required"), + ( + {"lat": 43.087}, + "lat and long must be given together", + "Pass both, e.g. lat=43.087, long=-89.509.", + ), ( {"lat": 43.087, "long": -89.509, "comid": 13294314}, - "comid cannot be provided with lat or long", + "lat/long and comid cannot be combined", + "one origin per call", ), ( {"lat": 43.087, "long": -89.509, "feature_source": "WQP"}, - "feature_source and feature_id cannot be provided with lat or long", + "lat/long and feature_source cannot be combined", + "one origin per call", + ), + ( + {"comid": 13294314}, + "navigation_mode is required", + "Pass one of 'UM', 'DM', 'UT', 'DD'.", ), - ({"comid": 13294314}, "navigation_mode is required"), ], ) -def test_get_features_rejects_ambiguous_origins(kwargs, message): - """Origin validation remains ahead of request execution after extraction.""" - with pytest.raises(ValueError, match=message): +def test_get_features_rejects_ambiguous_origins(kwargs, problem, remedy): + """Origin validation runs ahead of the request, and names the way out. + + Both halves are asserted because the caller is usually a program: the + problem alone tells it something is wrong, and only the remedy tells it + what to send instead. + """ + with pytest.raises(ValueError) as excinfo: get_features(**kwargs) + assert problem in str(excinfo.value) + assert remedy in str(excinfo.value) def test_get_features_includes_stop_comid(httpx_mock): @@ -388,6 +407,52 @@ def test_search_flowlines_without_navigation_mode_raises_value_error(): search(comid=13294314, find="flowlines") +@pytest.mark.parametrize( + ("kwargs", "problem"), + [ + ({}, "feature_source is required for find='basin'"), + ( + {"feature_source": "WQP"}, + "feature_source and feature_id must be given together", + ), + ({"feature_id": "USGS-01031500"}, "must be given together"), + ], +) +def test_search_for_basin_names_the_missing_half(kwargs, problem): + """An incomplete basin origin says which argument to add, and shows one. + + Covers both ways the pair can be incomplete -- neither supplied, and one + of the two -- because a caller that has to guess which it hit cannot + correct the call from the message alone. + """ + with pytest.raises(ValueError) as excinfo: + search(find="basin", **kwargs) + message = str(excinfo.value) + assert problem in message + assert "feature_source='WQP', feature_id='USGS-01031500'" in message + + +@pytest.mark.parametrize( + ("half", "supplied"), + [ + ("feature_id", {"feature_source": None, "feature_id": "USGS-01031500"}), + ("feature_source", {"feature_source": "WQP", "feature_id": None}), + ], +) +def test_half_a_feature_pair_beside_a_comid_is_reported_as_a_conflict(half, supplied): + """Completing the pair would only raise the origin conflict next. + + The pair check fired first, so the caller was told to supply the missing + half, and the corrected call then failed on ``comid`` -- two round trips + for one mistake. + """ + with pytest.raises(ValueError) as excinfo: + _validate_feature_source_comid(comid=13294314, **supplied) + message = str(excinfo.value) + assert f"comid and {half} cannot be combined" in message + assert "Pass both" not in message + + def test_validate_navigation_mode_raises_value_error_for_invalid(): """Regression: previously raised TypeError; should be ValueError.""" with pytest.raises(ValueError, match="Invalid navigation_mode"): @@ -473,3 +538,119 @@ def test_a_configured_base_url_redirects_every_nldi_request(httpx_mock): assert isinstance(gdf, GeoDataFrame) assert {str(r.url).startswith(mirror) for r in httpx_mock.get_requests()} == {True} + + +@pytest.mark.parametrize( + "kwargs", + [ + {"comid": 13294314, "navigation_mode": "UM"}, + { + "feature_source": "WQP", + "feature_id": "USGS-054279485", + "navigation_mode": "UM", + }, + ], +) +def test_navigation_without_a_data_source_says_what_to_add(kwargs, monkeypatch): + """A navigation needs the source naming which features to return. + + Without this the missing source was interpolated into the path as the + literal string 'None'; the service answered 200 with an empty + FeatureCollection and the caller got an empty GeoDataFrame with no way to + tell it apart from a navigation that really has nothing on it. + """ + # Seed the catalog: the feature_source case validates it on the way past, + # and the autouse fixture clears it, so an unseeded run reaches the network + # for a failure that is purely local. + monkeypatch.setattr(nldi, "_AVAILABLE_DATA_SOURCES", ["WQP", "nwissite"]) + with pytest.raises(ValueError) as excinfo: + get_features(**kwargs) + message = str(excinfo.value) + assert "data_source is required" in message + assert "data_source='nwissite'" in message + + +def test_a_bad_navigation_mode_is_reported_before_the_missing_data_source(): + """Both arguments are wrong; the mode is the one the caller typed. + + Requiring ``data_source`` ahead of validating the mode would answer a + mistyped ``navigation_mode`` with a message about a different argument, + so the caller fixes that, re-runs, and only then learns about the typo. + """ + with pytest.raises(ValueError) as excinfo: + get_features(comid=13294314, navigation_mode="XX") + assert "Invalid navigation_mode" in str(excinfo.value) + + +def test_get_features_by_data_source_returns_the_whole_catalog(httpx_mock): + """The one getter that takes no origin: every feature of a source.""" + mock_request_data_sources(httpx_mock) + mock_request( + httpx_mock, + f"{NLDI_API_BASE_URL}/WQP", + "tests/data/nldi_get_features_by_comid.json", + ) + + gdf = get_features_by_data_source("WQP") + + assert isinstance(gdf, GeoDataFrame) + assert not gdf.empty + + +def test_get_features_by_data_source_validates_the_source(httpx_mock): + mock_request_data_sources(httpx_mock) + with pytest.raises(ValueError, match="Invalid data source"): + get_features_by_data_source("not_a_real_source") + + +def test_a_200_with_a_non_json_body_becomes_an_empty_frame(httpx_mock): + """NLDI answers some queries 200 with an empty body, and that is not an + error condition -- a feature with nothing upstream is a real answer. + + This is the one place the package returns an empty frame rather than + raising on a malformed response. Pinned because it is deliberate: the + swallow is easy to mistake for an oversight and 'fix' into a raise, which + would turn a legitimate empty navigation into a crash. + """ + mock_request_data_sources(httpx_mock) + httpx_mock.add_response( + method="GET", + url=f"{NLDI_API_BASE_URL}/WQP", + text="", + headers={"Content-Type": "text/plain"}, + ) + + gdf = get_features_by_data_source("WQP") + + assert isinstance(gdf, GeoDataFrame) + assert gdf.empty + assert gdf.crs is not None # the CRS survives the empty path + + +def test_get_flowlines_forwards_stop_comid(httpx_mock): + """``stop_comid`` bounds a navigation and must reach the query string.""" + request_url = ( + f"{NLDI_API_BASE_URL}/comid/13294314/navigation/UM/flowlines" + "?distance=50&trimStart=false&stopComid=13294312" + ) + mock_request_data_sources(httpx_mock) + mock_request(httpx_mock, request_url, "tests/data/nldi_get_flowlines_by_comid.json") + + gdf = get_flowlines( + navigation_mode="UM", comid=13294314, distance=50, stop_comid=13294312 + ) + + assert isinstance(gdf, GeoDataFrame) + sent = httpx_mock.get_requests()[-1].url + assert "stopComid=13294312" in str(sent) + + +def test_search_rejects_a_basin_lookup_by_comid(): + """A basin is looked up by feature, not by flowline; the message must + offer both ways forward rather than only naming the conflict.""" + with pytest.raises(ValueError) as excinfo: + search(find="basin", comid=13294314) + message = str(excinfo.value) + assert "find='basin' cannot be combined with comid" in message + assert "feature_source" in message + assert "find='flowlines'" in message diff --git a/tests/nwdc_test.py b/tests/nwdc_test.py index c85544c47..b92ad58ca 100644 --- a/tests/nwdc_test.py +++ b/tests/nwdc_test.py @@ -16,7 +16,12 @@ from dataretrieval import configuration, nwdc from dataretrieval import progress as _progress from dataretrieval.exceptions import DataRetrievalError -from dataretrieval.nwdc import _next_page_url, _resolve_locations, get_wateruse +from dataretrieval.nwdc import ( + _next_page_url, + _nwdc_error_detail, + _resolve_locations, + get_wateruse, +) from dataretrieval.transport import fanout as _fanout from dataretrieval.utils import BaseMetadata @@ -404,6 +409,33 @@ def test_resolve_locations_empty_list_rejected(): _resolve_locations([], None, None) +def test_empty_selector_is_not_reported_as_the_wrong_selector_count(): + """One selector was given; the fault is its value, not how many there are. + + The message used to close with "exactly one of state, county, or huc must + be given", sending a caller who passed ``state=[]`` to change ``county`` + or ``huc`` instead of filling in ``state``. + """ + with pytest.raises(ValueError) as excinfo: + _resolve_locations([], None, None) + message = str(excinfo.value) + assert "Pass at least one state value." in message + assert "exactly one" not in message + + +def test_a_non_prose_detail_is_not_forwarded_into_the_message(): + """A validation envelope spells ``detail`` as a list of error objects.""" + response = httpx.Response(422, json={"detail": [{"loc": ["q"], "msg": "bad"}]}) + + assert _nwdc_error_detail(response) is None + + +def test_a_detail_that_already_ends_in_a_period_is_not_double_punctuated(): + response = httpx.Response(400, json={"detail": "Invalid model name: bad."}) + + assert "bad.. " not in _nwdc_error_detail(response) + + def test_resolve_locations_rejects_malformed_selectors(): with pytest.raises(ValueError): # unrecognized state _resolve_locations("Atlantis", None, None) diff --git a/tests/utils_test.py b/tests/utils_test.py index b461b019e..a648e5e0f 100644 --- a/tests/utils_test.py +++ b/tests/utils_test.py @@ -465,6 +465,65 @@ def test_resolves_an_iterable_element_wise(self): with pytest.raises(ValueError, match="not a recognized US state"): to_state(["WI", "XX"]) + def test_the_table_names_no_endpoint_parameter_of_its_own(self): + """``to_state`` is a pure conversion with no ``state`` argument and no + endpoint behind it, so its rejection must not tell a caller to pass a + differently-named parameter that this call does not accept.""" + from dataretrieval.codes.states import to_state + + with pytest.raises(ValueError) as excinfo: + to_state("Puerto Rico") + message = str(excinfo.value) + assert "no entry in this table" in message + assert "state_name" not in message + assert "state_code" not in message + + +class TestApplyStateTerritories: + """A territory has no row in the table, so the remedy is the endpoint's own + state queryable -- and *which* one differs per endpoint, so the message has + to be built where those names are known.""" + + def test_the_remedy_names_this_endpoints_native_parameters(self): + from dataretrieval.codes.states import apply_state + + with pytest.raises(ValueError) as excinfo: + apply_state( + {"state": "Puerto Rico"}, + to="name", + into="state_name", + reject=("state_code", "state_name"), + ) + message = str(excinfo.value) + assert "not a recognized US state" in message + assert "Pass state_name or state_code instead" in message + + def test_an_endpoint_with_no_alternative_offers_none(self): + """NGWMN's getters expose only the unified ``state``: ``sites`` + filters on the ``state_name`` queryable but does not accept it as an + argument, and ``providers``' queryable *is* ``state``. Appending a + remedy from ``into`` sent a caller to ``get_sites(state_name=...)`` + (``TypeError``) or straight back into this same error. + """ + from dataretrieval.codes.states import apply_state + + for into, to in (("state", "postal"), ("state_name", "name")): + with pytest.raises(ValueError) as excinfo: + apply_state({"state": "Guam"}, to=to, into=into) + message = str(excinfo.value) + assert "no entry in this table" in message + assert "instead" not in message + + def test_the_ngwmn_getters_reject_a_territory_without_misdirecting(self): + """End to end: whatever the message names must be an argument the + getter the caller actually called accepts.""" + from dataretrieval import ngwmn + + for getter in (ngwmn.get_sites, ngwmn.get_providers): + with pytest.raises(ValueError) as excinfo: + getter(state="Puerto Rico") + assert "state_name" not in str(excinfo.value) + def test_retrying_get_maps_invalid_url(monkeypatch): """Direct active-service GETs do not leak raw httpx InvalidURL errors.""" diff --git a/tests/validation_test.py b/tests/validation_test.py index 1f9f638a7..689112aaa 100644 --- a/tests/validation_test.py +++ b/tests/validation_test.py @@ -1,8 +1,20 @@ -"""Tests for the shared closed-vocabulary check.""" +"""Tests for the shared argument checks. + +Each check is asserted on two things: that it lets a valid call through, and +that its rejection names the move that would fix the call. The second half is +the point of the module -- a caller that is a program can only correct itself +from a message that says what to send instead. +""" import pytest -from dataretrieval._validation import require_one_of +from dataretrieval._validation import ( + reject_together, + require_argument, + require_exactly_one, + require_one_of, + require_together, +) def test_accepts_a_valid_option(): @@ -28,3 +40,92 @@ def test_a_string_vocabulary_is_refused(): fragment of a valid option. Refuse it at the one shared chokepoint.""" with pytest.raises(TypeError, match="not 'csv'"): require_one_of("cs", "csv", name="format") + + +class TestRequireArgument: + def test_accepts_a_supplied_value(self): + require_argument("navigation_mode", "UM") + + def test_accepts_a_falsy_but_supplied_value(self): + """``0`` and ``''`` were supplied; only ``None`` was not.""" + require_argument("distance", 0) + + def test_message_names_the_parameter_and_a_default_remedy(self): + with pytest.raises(ValueError) as excinfo: + require_argument("feature_id", None) + message = str(excinfo.value) + assert "feature_id is required" in message + assert "Pass a feature_id value." in message + + def test_context_says_what_made_it_required(self): + with pytest.raises(ValueError, match="when comid is given"): + require_argument("navigation_mode", None, context="when comid is given") + + def test_remedy_replaces_the_default(self): + with pytest.raises(ValueError, match="Pass one of 'UM', 'DM'."): + require_argument("navigation_mode", None, remedy="Pass one of 'UM', 'DM'.") + + +class TestRequireTogether: + def test_accepts_all_supplied(self): + require_together({"lat": 1.0, "long": 2.0}) + + def test_accepts_none_supplied(self): + """Declining the whole group is a different question from completing it.""" + require_together({"lat": None, "long": None}) + + def test_message_names_what_is_missing_and_what_to_do(self): + with pytest.raises(ValueError) as excinfo: + require_together({"lat": 1.0, "long": None}) + message = str(excinfo.value) + assert "lat and long must be given together" in message + assert "Missing: long" in message + assert "Pass long, or omit lat." in message + + def test_reports_every_missing_member_of_a_larger_group(self): + with pytest.raises(ValueError, match="Missing: b and c"): + require_together({"a": 1, "b": None, "c": None}) + + +class TestRequireExactlyOne: + def test_accepts_exactly_one(self): + require_exactly_one({"comid": 1, "feature_source": None}) + + def test_none_supplied_says_to_pass_one(self): + with pytest.raises(ValueError) as excinfo: + require_exactly_one({"state": None, "county": None, "huc": None}) + message = str(excinfo.value) + assert "Provide exactly one of state, county or huc" in message + assert "Supplied: none" in message + assert "Pass one of state, county or huc." in message + + def test_several_supplied_says_which_to_drop(self): + with pytest.raises(ValueError) as excinfo: + require_exactly_one({"state": "WI", "county": "55025", "huc": None}) + message = str(excinfo.value) + assert "Supplied: state and county" in message + assert "Drop all but one of state and county." in message + + +class TestRejectTogether: + def test_accepts_one_supplied(self): + reject_together({"lat": 1.0, "comid": None}) + + def test_accepts_none_supplied(self): + """Unlike require_exactly_one, an empty call is not this check's business.""" + reject_together({"lat": None, "comid": None}) + + def test_message_names_only_the_conflicting_arguments(self): + with pytest.raises(ValueError) as excinfo: + reject_together({"lat": 1.0, "comid": 2, "feature_source": None}) + message = str(excinfo.value) + assert "lat and comid cannot be combined" in message + # The argument that was never passed stays out of the remedy. + assert "feature_source" not in message + assert "Pass only one of lat or comid." in message + + def test_context_explains_why_they_conflict(self): + with pytest.raises(ValueError, match="-- they name different origins"): + reject_together( + {"lat": 1.0, "comid": 2}, context="they name different origins" + ) diff --git a/tests/waterdata_nearest_test.py b/tests/waterdata_nearest_test.py index e16e2470e..7a3362db5 100644 --- a/tests/waterdata_nearest_test.py +++ b/tests/waterdata_nearest_test.py @@ -415,3 +415,54 @@ def resume(self): assert list(resumed["target_time"]) == list(target) assert resumed_metadata is metadata + + +def test_caller_properties_keep_the_columns_the_match_needs(patch_get_continuous): + """A caller's ``properties`` list gains 'time' and the grouping column. + + Without the injection a list like ``['time', 'value']`` reached the + service unchanged, the response came back with no + ``monitoring_location_id``, and every site but one was silently dropped -- + a wrong answer with nothing for a caller to notice it by. + """ + patch_get_continuous.return_value = ( + pd.DataFrame( + [ + { + "time": "2023-06-15T10:30:00Z", + "value": 1.0, + "monitoring_location_id": "USGS-A", + }, + { + "time": "2023-06-15T10:30:00Z", + "value": 2.0, + "monitoring_location_id": "USGS-B", + }, + ] + ), + mock.Mock(), + ) + result, _ = get_nearest_continuous( + ["2023-06-15T10:30:31Z"], + monitoring_location_id=["USGS-A", "USGS-B"], + properties=["time", "value"], + ) + sent = patch_get_continuous.call_args.kwargs["properties"] + assert "monitoring_location_id" in sent + assert sent[:2] == ["time", "value"] + # One row per site, not one row for the pair. + assert len(result) == 2 + + +def test_properties_are_left_alone_when_already_complete(patch_get_continuous): + patch_get_continuous.return_value = ( + pd.DataFrame( + [{"time": "2023-06-15T10:30:00Z", "monitoring_location_id": "USGS-A"}] + ), + mock.Mock(), + ) + asked = ["time", "monitoring_location_id"] + get_nearest_continuous( + ["2023-06-15T10:30:31Z"], monitoring_location_id="USGS-A", properties=asked + ) + assert patch_get_continuous.call_args.kwargs["properties"] == asked diff --git a/tests/waterdata_test.py b/tests/waterdata_test.py index f6e859a4e..81887482e 100644 --- a/tests/waterdata_test.py +++ b/tests/waterdata_test.py @@ -1172,6 +1172,35 @@ def test_get_reference_table_rejects_unknown_collection_by_its_own_name(httpx_mo assert not httpx_mock.get_requests(), "must reject before issuing a request" +def test_get_reference_table_serves_countries(httpx_mock): + """``countries`` is a real reference collection and singularizes correctly. + + It sits beside ``counties`` in the service catalog but was missing from the + accepted vocabulary, so the rejection told a caller asking for a real + collection that it did not exist. The shared ``-s`` rule would also have + named its id column ``countrie``. + """ + _mock_items( + httpx_mock, + "countries", + body={ + "type": "FeatureCollection", + "features": [ + { + "id": "AD", + "type": "Feature", + "geometry": None, + "properties": {"id": "AD", "country_name": "Andorra"}, + } + ], + }, + ) + + df, _ = get_reference_table("countries") + + assert "country" in df.columns + + def test_get_reference_table_with_query(httpx_mock): """A ``query`` dict is merged into the request's query params.""" _mock_items(httpx_mock, "agency-codes") diff --git a/tests/waterservices_test.py b/tests/waterservices_test.py index 8de81122c..2674a0f55 100644 --- a/tests/waterservices_test.py +++ b/tests/waterservices_test.py @@ -24,37 +24,49 @@ def test_query_waterdata_validation(): - """Tests the validation parameters of the query_waterservices method""" + """Rejections name the filters, corners, and services that would work. + + Asserted on the remedy rather than the whole message: the caller is + typically a program, and what it needs from the failure is the set of + values that would have been accepted. + """ with pytest.raises(TypeError) as type_error: query_waterdata(service="pmcodes", format="rdb") - assert ( - str(type_error.value) - == "Query must specify a major filter: site_no, stateCd, bBox" - ) + message = str(type_error.value) + assert "Query must specify a major filter" in message + assert "site_no, stateCd" in message + assert "nw_longitude_va" in message with pytest.raises(TypeError) as type_error: query_waterdata(service=None, site_no="sites") - assert str(type_error.value) == "Service not recognized" + message = str(type_error.value) + assert "Unrecognized service: None" in message + # 'ratings' was advertised here but is not an NwisWeb program: the URL it + # built returned an HTML error page, not data. + assert "'peaks'" in message + assert "get_ratings" in message with pytest.raises(TypeError) as type_error: query_waterdata(service="pmcodes", nw_longitude_va="something") - assert ( - str(type_error.value) == "One or more lat/long coordinates missing or invalid." - ) + message = str(type_error.value) + assert "bounding box needs all four corners" in message + # The three corners actually absent, so the caller knows what to add. + assert "nw_latitude_va, se_longitude_va, se_latitude_va" in message def test_query_waterservices_validation(): """Tests the validation parameters of the query_waterservices method""" with pytest.raises(TypeError) as type_error: query_waterservices(service="dv", format="rdb") - assert ( - str(type_error.value) - == "Query must specify a major filter: sites, stateCd, bBox, huc, or countyCd" - ) + message = str(type_error.value) + assert "Query must specify a major filter" in message + assert "sites, stateCd, bBox, huc, countyCd" in message with pytest.raises(TypeError) as type_error: query_waterservices(service=None, sites="sites") - assert str(type_error.value) == "Service not recognized" + message = str(type_error.value) + assert "Unrecognized service: None" in message + assert "'dv', 'iv', 'site', 'stat'" in message def test_query_validation(httpx_mock): @@ -78,10 +90,22 @@ def test_query_validation(httpx_mock): def test_get_record_validation(): - """Tests the validation parameters of the get_record method""" + """An unknown service names the ones get_record does serve.""" with pytest.raises(TypeError) as type_error: get_record(sites=["01491000"], service="not_a_service") - assert str(type_error.value) == "Unrecognized service: not_a_service" + message = str(type_error.value) + assert "Unrecognized service: 'not_a_service'" in message + assert "'dv', 'iv', 'site', 'stat', 'peaks', 'ratings'" in message + + +def test_get_record_rejects_non_string_sites(): + """The type rejection shows both accepted spellings of ``sites``.""" + with pytest.raises(TypeError) as type_error: + get_record(sites=1491000, service="dv") + message = str(type_error.value) + assert "not int" in message + assert "sites='01491000'" in message + assert "sites=['01491000', '01645000']" in message def test_get_dv(httpx_mock): From 410ef0284bebf51704d16a414b8bfc77a4a2ffdd Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 21 Aug 2026 09:14:00 -0500 Subject: [PATCH 03/10] feat(cql): add a total-row cap to get_cql Expose the OGC engine's existing max_rows limit through get_cql so callers can cap total results rather than accidentally turning a small page size into hundreds of requests. --- dataretrieval/waterdata/cql.py | 14 ++++++++++++-- tests/waterdata_test.py | 20 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/dataretrieval/waterdata/cql.py b/dataretrieval/waterdata/cql.py index 2cc740c60..06829f267 100644 --- a/dataretrieval/waterdata/cql.py +++ b/dataretrieval/waterdata/cql.py @@ -45,6 +45,7 @@ def get_cql( properties: str | Iterable[str] | None = None, bbox: list[float] | None = None, limit: int | None = None, + max_rows: int | None = None, skip_geometry: bool | None = None, convert_type: bool = True, ) -> tuple[pd.DataFrame, BaseMetadata]: @@ -84,7 +85,16 @@ def get_cql( Bounding box ``[xmin, ymin, xmax, ymax]`` in CRS 4326. Combines with the CQL filter as an additional spatial predicate. limit : int, optional - Page size, clamped server-side to 50,000. + The number of features returned in each page, clamped server-side to + 50,000. This is a per-page size, not a cap on the total result: a + filter matching more rows than ``limit`` still returns every matching + row across multiple pages, so a small ``limit`` makes *more* requests, + not fewer. Use ``max_rows`` to cap the total instead. + max_rows : int, optional + Cap the total number of rows returned, stopping pagination early + instead of downloading the whole result. Unlike ``limit`` (the + per-page size), this bounds the total across every page. The default + (None) follows pagination to completion. skip_geometry : bool, optional If True, the server omits geometry from each feature (``skipGeometry=true``). @@ -165,7 +175,7 @@ def get_cql( "convert_type": convert_type, } ) - return get_ogc_data(args, collection, cql_body=body) + return get_ogc_data(args, collection, max_rows=max_rows, cql_body=body) __all__ = ["get_cql"] diff --git a/tests/waterdata_test.py b/tests/waterdata_test.py index 81887482e..b1c4ce7e6 100644 --- a/tests/waterdata_test.py +++ b/tests/waterdata_test.py @@ -1234,6 +1234,26 @@ def test_get_daily_max_rows_is_excluded_from_request_and_forwarded(): assert fake.call_args.kwargs["max_rows"] == 3 # forwarded to the cap +def test_get_cql_max_rows_is_excluded_from_request_and_forwarded(): + """``get_cql`` caps the total like every other Water Data getter. + + It was the only one without ``max_rows``, and its ``limit`` is the page + size -- so the obvious way to ask for a few rows instead paged the whole + match a few rows at a time. A bounded probe written that way spent ~400 + requests of an hourly quota of 1000 before the service refused it. + """ + with mock.patch("dataretrieval.waterdata.cql.get_ogc_data") as fake: + fake.return_value = (pd.DataFrame(), mock.MagicMock(spec=[])) + get_cql( + collection="daily", + cql={"op": "=", "args": [{"property": "parameter_code"}, "00060"]}, + max_rows=3, + ) + args_dict = fake.call_args[0][0] + assert "max_rows" not in args_dict # not leaked into the query params + assert fake.call_args.kwargs["max_rows"] == 3 # forwarded to the cap + + def test_get_reference_table_wrong_name(): with pytest.raises(ValueError): get_reference_table("agency-cod") From 77fd19d65c9b1ba471256f787b660826db562da1 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 21 Aug 2026 09:14:02 -0500 Subject: [PATCH 04/10] test: enforce a branch-coverage ratchet Enable branch coverage, close behaviorally meaningful gaps, exclude only generated or environment-unreachable paths, and set the threshold at the measured value so future regressions fail without encouraging hollow tests. --- .github/workflows/python-package.yml | 15 ++- AGENTS.md | 9 +- CONTRIBUTING.md | 21 +++- dataretrieval/waterdata/reference.py | 7 +- pyproject.toml | 41 ++++++++ tests/configuration_test.py | 125 +++++++++++++++++++++++ tests/nwis_test.py | 147 +++++++++++++++++++++++++++ tests/streamstats_test.py | 40 ++++++++ tests/transport_test.py | 32 ++++++ tests/utils_test.py | 55 ++++++++++ tests/waterdata_chunking_test.py | 34 +++++++ tests/waterdata_filters_test.py | 25 +++++ tests/waterdata_nearest_test.py | 79 ++++++++++++++ tests/waterdata_progress_test.py | 27 +++++ tests/waterdata_test.py | 9 ++ tests/waterdata_utils_test.py | 142 ++++++++++++++++++++++++++ 16 files changed, 801 insertions(+), 7 deletions(-) diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index a95ffec9e..07f4f5dca 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -53,6 +53,16 @@ jobs: - name: Dependency-direction contracts # Rules and rationale live in .importlinter and the ADRs it cites. run: lint-imports + - name: Coverage ratchet + # Graded here rather than in the OS/Python matrix: several tests are + # POSIX-only (``skipif``), so a Windows or macOS run measures a + # genuinely smaller suite and would trip a shared threshold. The + # matrix still reports its own coverage; this is the one that blocks. + # Threshold and rationale live in [tool.coverage.report]. + run: | + pip install -e .[test,nldi] + coverage run -m pytest tests/ + coverage report - name: Complexity trend vs base # Advisory: reports which files moved and by how much, so a reviewer # can see direction rather than a pass/fail. Never fails the build -- @@ -154,4 +164,7 @@ jobs: shell: bash run: | coverage run -m pytest tests/ - coverage report -m + # --fail-under=0 disables the ratchet here on purpose: this matrix + # skips POSIX-only tests on Windows, so its number is informational. + # The gate that blocks runs once, on Linux, in the complexity job. + coverage report -m --fail-under=0 diff --git a/AGENTS.md b/AGENTS.md index ff8645d4c..5dc5287ec 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -65,8 +65,13 @@ can predict where a thing lives. ## Commands - Lint: `ruff check .` and `ruff format --check .` (pinned to the version in `.pre-commit-config.yaml` and the CI lint job — keep them aligned). -- Tests: `coverage run -m pytest tests/ && coverage report -m`, or focused like - `pytest tests/waterdata_test.py::test_mock_get_samples`. +- Tests: `coverage run -m pytest tests/ && coverage report`, or focused like + `pytest tests/waterdata_test.py::test_mock_get_samples`. `coverage report` is + a merge gate: branch coverage with a `fail_under` ratchet in + `[tool.coverage.report]`. Chase the uncovered *branch*, not the number -- a + test written to colour a line green catches nothing and costs a maintenance + slot. If a path is genuinely unreachable, add it to `exclude_also` with a + reason, or leave the ratchet alone. - Types: `mypy` (`strict = true` in `pyproject.toml`; CI runs it over the PR-merged-into-main, so bare `dict`/`list` annotations fail there even if they pass on your branch). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4416e8d76..1ff3d8c66 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -133,7 +133,7 @@ ruff check . ruff format --check . mypy coverage run -m pytest tests/ -coverage report -m +coverage report xenon --max-absolute C --max-modules B --max-average A dataretrieval complexipy dataretrieval lint-imports @@ -142,6 +142,25 @@ lint-imports The last three come from `pip install -e '.[metrics]'`, and each has a pre-commit hook running the identical check, so a clean pre-commit run means CI agrees. +`coverage report` is a ratchet too. The threshold lives in +`[tool.coverage.report]` in `pyproject.toml` and sits at the measured value, so +it fails on regression rather than demanding new tests of a change that added +none. Raise it when coverage rises; lower it only deliberately, and say why in +the commit. + +Coverage is measured with branches on, because most of what this package gets +wrong is a branch rather than a line -- a dispatch arm routing to the wrong +getter, an error path that never fires, a fallback that quietly becomes the +norm. Chase the *uncovered branch*, not the percentage: a test written only to +colour a line green costs a real maintenance slot and catches nothing. If a +path cannot be reached without contorting the code, exclude it in +`[tool.coverage.report] exclude_also` with a reason, or leave the ratchet where +it is. Both are better than a hollow test. + +The blocking run is a single Linux job. The OS/Python matrix reports its own +number with `--fail-under=0`, because several tests are POSIX-only and a +Windows run genuinely measures a smaller suite. + `xenon` and `complexipy` are complexity ratchets: the thresholds are the tightest the package passes today, so they fail only when a change makes things worse. They disagree usefully. `xenon` counts branches (cyclomatic complexity), diff --git a/dataretrieval/waterdata/reference.py b/dataretrieval/waterdata/reference.py index b5ab18fdb..f5ea78f01 100644 --- a/dataretrieval/waterdata/reference.py +++ b/dataretrieval/waterdata/reference.py @@ -95,12 +95,13 @@ def get_reference_table( require_one_of(collection, get_args(METADATA_COLLECTIONS), name="collection") # Give the ID column the collection name, singularized and underscored. + # ``removesuffix`` rather than an ``endswith`` branch: every collection in + # the vocabulary is plural today, so the non-plural arm was unreachable, + # and this stays correct if a singular one is ever added. if collection in ("counties", "countries"): output_id = collection[:-3] + "y" # county / country - elif collection.endswith("s"): - output_id = collection[:-1].replace("-", "_") else: - output_id = collection.replace("-", "_") + output_id = collection.removesuffix("s").replace("-", "_") query_args = dict(query) if query else {} if limit is not None: diff --git a/pyproject.toml b/pyproject.toml index 54d8ab34a..eac78442b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -183,3 +183,44 @@ addopts = "-m 'not live'" markers = [ "live: hits real USGS services; deselected by default, run on a schedule", ] + +[tool.coverage.run] +source = ["dataretrieval"] +# Branch coverage, because most of what this package gets wrong is a branch +# rather than a line: a service-dispatch arm that routes to the wrong getter, +# an error path that never fires, a fallback that silently becomes the norm. +branch = true +omit = [ + # Written by setuptools_scm at build time. Not ours, and absent from a + # source checkout until something builds. + "dataretrieval/_version.py", +] + +[tool.coverage.report] +show_missing = true +skip_covered = true +# Two decimals so the gate compares against the real number rather than a +# rounded one -- 98.97% displays as "99%" and would otherwise pass a +# ``fail_under = 99`` on rounding alone. +precision = 2 +# The ratchet, set at the measured value. Raise it when coverage rises; never +# lower it to make a red build green -- lower it only deliberately, when a +# genuinely untestable path is added and excluding it would be dishonest. +# +# The remaining gap is concentrated in async fan-out internals +# (transport/fanout, ogc/engine), the response-shaping dialect defaults +# (ogc/shaping), and a handful of partial branches. Those are reachable, but +# each needs real scaffolding rather than a one-liner -- they are the next +# rungs, not exclusions. +fail_under = 98.9 +exclude_also = [ + # Type-checking-only imports never execute. + "if TYPE_CHECKING:", + "@overload", + # Environment fallbacks for a dependency or a package that is absent. The + # suite installs the [test,nldi] extras, so these branches are unreachable + # here by construction; reaching them means unimporting a module mid-run, + # which tests the import system rather than this package. + "except ImportError", + "except PackageNotFoundError", +] diff --git a/tests/configuration_test.py b/tests/configuration_test.py index 609d16456..e72e52220 100644 --- a/tests/configuration_test.py +++ b/tests/configuration_test.py @@ -17,6 +17,7 @@ import pytest import dataretrieval +from dataretrieval import _configuration_core as _core from dataretrieval import configuration, streamstats, waterdata from dataretrieval.configuration import Configuration from dataretrieval.ngwmn import NgwmnConfiguration @@ -2024,3 +2025,127 @@ def test_show_configuration_survives_a_malformed_profile(config_file): WaterdataConfiguration.load("bulk") with pytest.raises(configuration.ConfigurationError, match="contains a table"): NgwmnConfiguration.load("gentle") + + +class TestConfigValueParsing: + """The coercion layer between a config file / env var and a setting. + + Every one of these is a message a user reads while their config is not + working, so each names the setting, what it expected, and what it got. + """ + + def test_a_non_numeric_stall_timeout_is_rejected_by_type(self): + with pytest.raises(configuration.ConfigurationError) as excinfo: + _core._coerce_seconds("soon", "stall_timeout", "") + message = str(excinfo.value) + assert "stall_timeout" in message + assert "a number of seconds" in message + + def test_a_bool_is_not_a_number_of_seconds(self): + """``True`` is an ``int`` in Python, so a bare isinstance check would + accept ``stall_timeout = true`` and silently mean one second.""" + with pytest.raises(configuration.ConfigurationError): + _core._coerce_seconds(True, "stall_timeout", "") + + def test_a_blank_count_falls_through_to_the_default(self): + """An empty env var means "unset", not "zero" -- exporting an empty + string is how a shell unsets a variable in practice.""" + assert _core._parse_int(" ", "API_USGS_RETRIES", default=4, minimum=0) == 4 + + def test_a_blank_stall_timeout_falls_through_to_the_default(self): + assert ( + _core._parse_seconds(" ", "API_USGS_STALL_TIMEOUT") + == _core.DEFAULT_STALL_TIMEOUT + ) + + def test_a_blank_progress_toggle_is_refused_in_strict_mode(self): + """A config file is strict: a blank value there is a typo, not an + unset. The env path stays permissive for backwards compatibility.""" + with pytest.raises(configuration.ConfigurationError, match="must not be blank"): + _core._parse_progress("", "progress", strict=True) + + def test_an_adapter_key_that_is_not_a_table_says_what_it_should_be( + self, config_file + ): + """``[nldi]`` names an adapter, so ``nldi = 4`` at top level is a + caller who meant a table; the message must say so rather than + reporting an unknown setting.""" + config_file("nldi = 4\n") + with pytest.raises(configuration.ConfigurationError) as excinfo: + configuration.retries() + message = str(excinfo.value) + assert "[nldi]" in message + assert "table of settings" in message + + +class TestConfigPathResolutionFailures: + """The file layer sits on the per-request path, so a filesystem that will + not answer must not take every query down with it.""" + + def test_an_unresolvable_home_leaves_the_file_layer_inert(self, monkeypatch): + """A container with no passwd entry raises from ``Path.home()``. The + unexpanded ``~`` form is returned instead: it does not exist, so the + file layer is simply empty, and the environment alone still works -- + which is how this package behaved before settings were layered.""" + monkeypatch.setattr( + _core.Path, + "home", + staticmethod(lambda: (_ for _ in ()).throw(RuntimeError)), + ) + assert _core._default_home_path() == pathlib.Path( + "~/.dataretrieval/config.toml" + ) + + def test_a_missing_working_directory_is_a_configuration_error(self, monkeypatch): + """A job that deletes its own cwd cannot resolve a relative + DATARETRIEVAL_CONFIG. That must surface as this module's own error + type rather than a bare OSError escaping onto the request path.""" + monkeypatch.setattr( + _core.Path, + "cwd", + staticmethod(lambda: (_ for _ in ()).throw(OSError("gone"))), + ) + with pytest.raises(configuration.ConfigurationError) as excinfo: + _core._resolve_against_cwd(pathlib.Path("config.toml")) + message = str(excinfo.value) + assert "working directory is unavailable" in message + assert _core.CONFIG_PATH_ENV in message + + def test_an_unreadable_config_file_names_the_path(self, tmp_path): + missing = tmp_path / "nope.toml" + with pytest.raises(configuration.ConfigurationError, match="could not read"): + _core._read_file_content(missing) + + def test_the_home_memo_watches_the_variable_that_moves_the_path(self, monkeypatch): + """``ntpath.expanduser`` ignores HOME and reads USERPROFILE, so on + Windows the memo must watch USERPROFILE or it invalidates on a + variable that cannot move the path and misses the one that can.""" + monkeypatch.setattr(_core.os, "name", "nt") + monkeypatch.setenv("USERPROFILE", r"C:\Users\ada") + monkeypatch.setenv("HOME", "/ignored") + assert _core._home_id() == r"C:\Users\ada" + + monkeypatch.delenv("USERPROFILE") + monkeypatch.setenv("HOMEDRIVE", "C:") + monkeypatch.setenv("HOMEPATH", r"\Users\ada") + assert _core._home_id() == r"C:\Users\ada" + + +def test_show_configuration_reports_an_unresolvable_path_as_the_file_row( + monkeypatch, +): + """A caller runs ``show_configuration`` precisely when their config is not + behaving. If path resolution itself fails, raising out of the explainer + withholds the one answer they came for.""" + monkeypatch.setattr( + configuration, + "config_path", + lambda: (_ for _ in ()).throw( + configuration.ConfigurationError("working directory is unavailable") + ), + ) + out = io.StringIO() + dataretrieval.show_configuration(stream=out) + text = out.getvalue() + assert "config file None: ServiceInterrupted(completed_chunks=0, total_chunks=1, cause=first).status_code is None ) + + +def test_an_unusable_next_page_link_is_reported_not_swallowed(): + """A malformed ``next`` href ends the page walk, so it must raise rather + than quietly truncate the result -- a short frame with no error is + indistinguishable from a complete one.""" + from dataretrieval.transport.links import resolve_next_url + + response = httpx.Response( + 200, request=httpx.Request("GET", "https://api.waterdata.usgs.gov/x") + ) + with pytest.raises(exceptions.DataRetrievalError) as excinfo: + resolve_next_url(None, response, service="Water Data") + message = str(excinfo.value) + assert "unusable next-page link" in message + assert "page walk cannot continue" in message + + +class TestErrorForStatus: + def test_a_success_status_is_a_usage_error(self): + """``error_for_status`` builds an exception for a failure. Handing it a + 200 means the caller's branch is wrong, and returning some default + exception would hide that.""" + with pytest.raises(ValueError, match="expects an HTTP error status"): + exceptions.error_for_status(200, "not an error") + + def test_a_leaf_without_a_default_status_demands_one(self): + """Only RateLimited and ServiceUnavailable imply their own status. Any + other HTTPError constructed without one would carry a meaningless + status_code, so it refuses instead.""" + with pytest.raises(TypeError, match="requires status_code"): + exceptions.TransientError("boom") diff --git a/tests/utils_test.py b/tests/utils_test.py index a648e5e0f..ca74c7a34 100644 --- a/tests/utils_test.py +++ b/tests/utils_test.py @@ -2,6 +2,7 @@ from unittest import mock +import numpy import pandas as pd import pytest @@ -537,3 +538,57 @@ def test_retrying_get_maps_invalid_url(monkeypatch): with pytest.raises(exceptions.URLTooLong): _querying._get_with_retry("https://example.invalid") + + +class TestFormatDatetime: + """``format_datetime`` joins the three columns NWIS RDB splits a + timestamp across, and is the only place the package parses a local time + with a named zone.""" + + def test_joins_date_time_and_zone_into_utc(self): + df = pd.DataFrame( + { + "sample_dt": ["2018-01-24", "2018-06-24"], + "sample_tm": ["10:30", "10:30"], + "sample_tz_cd": ["EST", "EDT"], + } + ) + + out = utils.format_datetime(df, "sample_dt", "sample_tm", "sample_tz_cd") + + assert str(out["datetime"].dt.tz) == "UTC" + # EST is -0500 and EDT -0400, so the same wall clock is a different + # instant in each row -- the reason the zone column cannot be ignored. + assert out["datetime"][0].hour == 15 + assert out["datetime"][1].hour == 14 + + def test_warns_and_keeps_going_when_a_timestamp_will_not_parse(self): + """An unparseable row becomes NaT rather than failing the whole frame, + but silently dropping timestamps would be a wrong answer -- so it + warns, and names the switch that avoids the loss.""" + df = pd.DataFrame( + { + # A missing date field, as an RDB row with an unrecorded + # sample date arrives. There is no ``errors="coerce"``, so a + # malformed *string* raises; only an absent value reaches here. + "sample_dt": ["2018-01-24", numpy.nan], + "sample_tm": ["10:30", "10:30"], + "sample_tz_cd": ["EST", "EST"], + } + ) + + with pytest.warns(UserWarning, match="incomplete dates"): + out = utils.format_datetime(df, "sample_dt", "sample_tm", "sample_tz_cd") + + assert out["datetime"].isna().sum() == 1 + assert out["datetime"].notna().sum() == 1 + + +def test_base_metadata_repr_names_the_type_and_url(): + """``md`` is what a user prints when a query surprises them, so the repr + has to say which metadata class it is and which URL produced it.""" + response = mock.MagicMock() + response.url = "https://example.test/items?limit=1" + md = utils.BaseMetadata(response) + assert "BaseMetadata" in repr(md) + assert "https://example.test/items?limit=1" in repr(md) diff --git a/tests/waterdata_chunking_test.py b/tests/waterdata_chunking_test.py index beb7a908c..7222c9af5 100644 --- a/tests/waterdata_chunking_test.py +++ b/tests/waterdata_chunking_test.py @@ -2539,3 +2539,37 @@ async def fetch(args): fetch({"monitoring_location_id": sites}) assert len(calls) == n assert sum(calls) == 8 + + +class TestSetResponseUrl: + """The combined response advertises the canonical URL, not the last + chunk's. ``httpx.Response`` resolves ``.url`` through its bound request, + so the rebind has to go through the request rather than the attribute.""" + + def test_a_response_with_no_bound_request_gets_one_synthesized(self): + """``httpx.Response(200)`` built by hand has no request, and reading + ``.request`` raises rather than returning None.""" + from dataretrieval.combining import _set_response_url + + response = httpx.Response(200) + _set_response_url(response, "https://example.test/combined") + + assert str(response.url) == "https://example.test/combined" + + def test_an_existing_request_keeps_its_method_and_headers(self): + """A combined POST must not silently become a GET, and the headers + carry the credential scoping.""" + from dataretrieval.combining import _set_response_url + + original = httpx.Request( + "POST", "https://example.test/chunk1", headers={"X-Api-Key": "k"} + ) + response = httpx.Response(200, request=original) + + _set_response_url(response, "https://example.test/combined") + + assert response.request.method == "POST" + assert response.request.headers["X-Api-Key"] == "k" + assert str(response.url) == "https://example.test/combined" + # A shallow copy sharing the old request must not see the new URL. + assert str(original.url) == "https://example.test/chunk1" diff --git a/tests/waterdata_filters_test.py b/tests/waterdata_filters_test.py index ff6c6f250..774fbf169 100644 --- a/tests/waterdata_filters_test.py +++ b/tests/waterdata_filters_test.py @@ -445,3 +445,28 @@ def test_get_continuous_surfaces_pitfall_to_caller(): filter_lang="cql-text", ) build.assert_not_called() + + +class TestOrSeparatorBoundaries: + """Top-level ``OR`` splitting drives chunking, so a false split changes + the query's meaning and a missed one leaves an unchunkable filter.""" + + def test_a_word_merely_starting_with_or_is_not_a_separator(self): + """``A ORDER BY b`` must not split on ``OR``; the trailing space is + what distinguishes the keyword from a longer identifier.""" + from dataretrieval.ogc.filters import _resume_after_or + + assert _resume_after_or("a ORDER BY b", 1) is None + + def test_a_trailing_or_at_end_of_expression_is_not_a_separator(self): + """There is no clause after it, so treating it as one would index + past the end.""" + from dataretrieval.ogc.filters import _resume_after_or + + assert _resume_after_or("a OR", 1) is None + + def test_a_real_separator_returns_the_next_clause_start(self): + from dataretrieval.ogc.filters import _resume_after_or + + expr = "a OR b" + assert expr[_resume_after_or(expr, 1) :] == "b" diff --git a/tests/waterdata_nearest_test.py b/tests/waterdata_nearest_test.py index 7a3362db5..55a0a92ed 100644 --- a/tests/waterdata_nearest_test.py +++ b/tests/waterdata_nearest_test.py @@ -466,3 +466,82 @@ def test_properties_are_left_alone_when_already_complete(patch_get_continuous): ["2023-06-15T10:30:31Z"], monitoring_location_id="USGS-A", properties=asked ) assert patch_get_continuous.call_args.kwargs["properties"] == asked + + +def test_no_observation_inside_the_window_returns_the_empty_shape( + patch_get_continuous, +): + """A target with nothing near it is a legitimate answer, not a failure -- + but the frame must keep the result columns so a caller can concatenate it + with a populated one instead of special-casing empties.""" + patch_get_continuous.return_value = ( + pd.DataFrame( + [ + { + "time": "2023-06-15T10:30:00Z", + "value": 1.0, + "monitoring_location_id": "A", + } + ] + ), + mock.Mock(), + ) + result, _ = get_nearest_continuous( + ["2020-01-01T00:00:00Z"], # years from the only observation + monitoring_location_id="A", + window="1h", + ) + assert result.empty + assert "target_time" in result.columns + + +class TestNearestPartialResults: + """When a fan-out is interrupted the caller still gets the chunks that + finished, shaped like a normal result -- otherwise recovering from an + interruption means handling a second frame layout.""" + + def test_a_partial_frame_with_no_completed_chunks_keeps_the_result_shape(self): + from dataretrieval.waterdata.nearest import _NearestSelector + + selector = _NearestSelector( + pd.to_datetime(["2023-06-15T10:30:00Z"]), pd.Timedelta("1h"), "first" + ) + out = selector.select_partial(pd.DataFrame()) + + assert out.empty + assert "target_time" in out.columns + + def test_a_partial_frame_with_rows_is_selected_normally(self): + from dataretrieval.waterdata.nearest import _NearestSelector + + selector = _NearestSelector( + pd.to_datetime(["2023-06-15T10:30:00Z"]), pd.Timedelta("1h"), "first" + ) + frame = pd.DataFrame( + [ + { + "time": "2023-06-15T10:30:05Z", + "value": 1.0, + "monitoring_location_id": "A", + } + ] + ) + out = selector.select_partial(frame) + + assert len(out) == 1 + + def test_the_wrapper_passes_the_inner_calls_live_response_through(self): + """``partial_response`` is how a caller inspects what arrived before + the interruption; the nearest wrapper must not shadow it.""" + from dataretrieval.waterdata.nearest import _NearestCall, _NearestSelector + + inner = mock.Mock() + inner.partial_response = "sentinel-response" + call = _NearestCall( + inner, + _NearestSelector( + pd.to_datetime(["2023-06-15T10:30:00Z"]), pd.Timedelta("1h"), "first" + ), + ) + + assert call.partial_response == "sentinel-response" diff --git a/tests/waterdata_progress_test.py b/tests/waterdata_progress_test.py index 9179f1b35..b05ba8f2a 100644 --- a/tests/waterdata_progress_test.py +++ b/tests/waterdata_progress_test.py @@ -567,3 +567,30 @@ async def run(): # current_chunk reflects the total number of successful chunks — # plan.total in the all-success case. assert current_recorded == plan.total + + +def test_closing_twice_is_a_no_op(): + """``close`` runs from both the success path and the interruption + handler, so a call that fails after finishing would close twice and + print a second trailing newline into the user's terminal.""" + stream = io.StringIO() + reporter = _progress.ProgressReporter(enabled=True, stream=stream) + reporter._rendered = True + reporter.close() + first = stream.getvalue() + reporter.close() + assert stream.getvalue() == first + + +def test_a_broken_stream_disables_the_reporter_instead_of_failing_the_query(): + """Progress is decoration. A closed or redirected stream must not take + down a query whose data already arrived.""" + + class _Broken(io.StringIO): + def write(self, s): + raise ValueError("stream closed") + + reporter = _progress.ProgressReporter(enabled=True, stream=_Broken()) + reporter._rendered = True + reporter.close() + assert reporter.enabled is False diff --git a/tests/waterdata_test.py b/tests/waterdata_test.py index b1c4ce7e6..da17d968a 100644 --- a/tests/waterdata_test.py +++ b/tests/waterdata_test.py @@ -1499,3 +1499,12 @@ def test_list_of_ints_rejected_at_boundary(self): monitoring_location_id="USGS-05427718", parameter_code=[60, 65], ) + + +def test_get_reference_table_forwards_limit_as_a_query_arg(): + """``limit`` is a server-side page size and belongs in the query, unlike + ``max_rows``, which is a client-side cap the service never sees.""" + with mock.patch("dataretrieval.waterdata.reference.get_ogc_data") as fake: + fake.return_value = (pd.DataFrame(), mock.MagicMock(spec=[])) + get_reference_table("agency-codes", limit=25) + assert fake.call_args.kwargs["args"]["limit"] == 25 diff --git a/tests/waterdata_utils_test.py b/tests/waterdata_utils_test.py index 27e4b9b79..507b2b933 100644 --- a/tests/waterdata_utils_test.py +++ b/tests/waterdata_utils_test.py @@ -903,6 +903,52 @@ def test_format_api_dates(value, date, expected): assert _format_api_dates(value, date=date) == expected +def test_format_api_dates_passes_none_through(): + """``None`` means "no date filter", not an empty one.""" + assert _format_api_dates(None) is None + + +def test_format_api_dates_treats_an_all_blank_sequence_as_no_filter(): + """``[None, None]`` is an interval with no endpoints, which is no filter at + all -- sending ``../..`` would be a query the service has to reject.""" + assert _format_api_dates([None, None]) is None + assert _format_api_dates(["", ""]) is None + + +def test_format_api_dates_rejects_more_than_two_values(): + """A date filter is an instant, a duration, or a closed interval. Three + values is a caller who meant something else, and the message says which + shapes exist rather than truncating silently.""" + with pytest.raises(ValueError) as excinfo: + _format_api_dates(["2024-01-01", "2024-06-01", "2024-12-31"], name="time") + message = str(excinfo.value) + assert "time takes at most 2 values, got 3" in message + assert "closed interval" in message + + +def test_the_duration_example_is_withheld_where_durations_are_rejected(): + """``get_ratings`` refuses ISO 8601 durations, so the shared message must + not offer 'P7D' on that path -- a caller following it would be sent + straight into a second rejection.""" + with pytest.raises(ValueError) as allowed: + _format_api_dates(["a", "b", "c"], name="time") + with pytest.raises(ValueError) as refused: + _format_api_dates(["a", "b", "c"], name="time", allow_duration=False) + + assert "'P7D'" in str(allowed.value) + assert "'P7D'" not in str(refused.value) + assert "an instant ('2020-01-01')" in str(refused.value) + + +def test_format_api_dates_names_the_callers_parameter(): + """The subject must be the argument the caller passed. It used to be + ``datetime_input`` -- a local of this private helper that no public getter + accepts, so correcting the argument the message named sent an + unrecognized parameter.""" + with pytest.raises(TypeError, match="^time must be a string"): + _format_api_dates({"2024-01-01": "ignored"}, name="time") + + def test_format_api_dates_rejects_mapping(): """`time={"2024-01-01": "x"}` would silently materialize as the keys list, accepting input the user clearly didn't intend. @@ -1345,3 +1391,99 @@ def test_credential_shaped_queryables_are_rejected(name): ) def test_real_queryables_still_pass_through(name): assert _flatten_queryables({"queryables": {name: "v"}}) == {name: "v"} + + +class TestWireIdSwitch: + """The API keys every collection on ``id``; callers spell it after the + collection (``monitoring_location_id``). The switch happens here, and + dropping the wrong key silently sends an unfiltered query.""" + + def test_the_collection_scoped_spelling_becomes_id(self): + from dataretrieval.ogc.requests import _switch_arg_id + + # The collection-derived spelling wins over the getter's own id_name. + out = _switch_arg_id( + {"monitoring_locations_id": "USGS-01646500"}, + "some_other_id", + "monitoring-locations", + ) + assert out == {"id": "USGS-01646500"} + + def test_the_getters_own_id_name_becomes_id(self): + """A getter whose id argument is not the collection name + ``_id`` + still has its spelling translated.""" + from dataretrieval.ogc.requests import _switch_arg_id + + out = _switch_arg_id({"site_id": "X"}, "site_id", "daily") + assert out == {"id": "X"} + + def test_an_explicit_id_wins_and_the_aliases_are_dropped(self): + from dataretrieval.ogc.requests import _switch_arg_id + + out = _switch_arg_id( + {"id": "chosen", "daily_id": "ignored"}, "daily_id", "daily" + ) + assert out == {"id": "chosen"} + + +def test_extract_features_returns_none_for_a_missing_body(): + """``None`` means "give the caller an empty frame". An empty features list + is a real mid-pagination shape, and letting it through would crash the + downstream merge with a missing join key rather than returning nothing.""" + from dataretrieval.waterdata.stats import _extract_features + + assert _extract_features(None) is None + assert _extract_features({"features": []}) is None + assert _extract_features({"features": [{"id": 1}]}) == [{"id": 1}] + + +def test_next_req_url_parses_the_body_when_not_handed_one(): + """The page walk normally passes the already-parsed body to avoid a second + parse; the no-body path is what a caller outside the walk gets, and it must + still find the link rather than returning None and truncating the walk.""" + from dataretrieval.ogc.engine import _next_req_url + + payload = { + "features": [{"id": 1}], + "links": [{"rel": "next", "href": "https://example.test/page2"}], + } + response = httpx.Response( + 200, json=payload, request=httpx.Request("GET", "https://example.test/page1") + ) + + assert _next_req_url(response) == "https://example.test/page2" + + +def test_next_req_url_stops_on_a_page_with_no_features(): + """A ``next`` link on a featureless page is the service's pagination + running past the end; following it would loop.""" + from dataretrieval.ogc.engine import _next_req_url + + payload = { + "features": [], + "links": [{"rel": "next", "href": "https://example.test/page2"}], + } + response = httpx.Response( + 200, json=payload, request=httpx.Request("GET", "https://example.test/page1") + ) + + assert _next_req_url(response) is None + + +class TestOgcJsonErrorDetail: + """The service's own wording is surfaced when it sends one; anything else + must fall back to the status-derived message rather than raising while + building an error.""" + + def test_a_non_json_body_yields_no_detail(self): + from dataretrieval.ogc.errors import _json_error_detail + + resp = httpx.Response(400, text="gateway") + assert _json_error_detail(resp) is None + + def test_a_json_scalar_body_yields_no_detail(self): + """A bare string or list is valid JSON but carries no error envelope.""" + from dataretrieval.ogc.errors import _json_error_detail + + assert _json_error_detail(httpx.Response(400, json="just a string")) is None + assert _json_error_detail(httpx.Response(400, json=[1, 2, 3])) is None From a1180860a4ca2de5c0e88bff62f862f7a01dd0b8 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 21 Aug 2026 09:14:05 -0500 Subject: [PATCH 05/10] fix: align branch documentation with behavior Correct stale service and collection documentation, make the date-formatting hint parameter describe rather than imply enforcement, and document how to interpret the coverage threshold when local platform skips differ from CI. --- CONTRIBUTING.md | 6 ++++++ dataretrieval/nwis.py | 4 +++- dataretrieval/ogc/dates.py | 16 ++++++++++++++-- dataretrieval/waterdata/ratings.py | 2 +- dataretrieval/waterdata/reference.py | 8 +++++--- pyproject.toml | 12 +++++++++--- tests/waterdata_test.py | 15 +++++++++++++++ tests/waterdata_utils_test.py | 2 +- 8 files changed, 54 insertions(+), 11 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1ff3d8c66..3b5af121c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -161,6 +161,12 @@ The blocking run is a single Linux job. The OS/Python matrix reports its own number with `--fail-under=0`, because several tests are POSIX-only and a Windows run genuinely measures a smaller suite. +For the same reason the threshold assumes the whole suite: on Windows, or +without the `nldi` extra installed, some tests skip and the local number comes +in under the gate through no fault of your change. Run +`coverage report --fail-under=0` in that situation and let CI grade the +ratchet. + `xenon` and `complexipy` are complexity ratchets: the thresholds are the tightest the package passes today, so they fail only when a change makes things worse. They disagree usefully. `xenon` counts branches (cyclomatic complexity), diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index b1fdac39a..56c552516 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -372,7 +372,9 @@ def query_waterdata( Parameters ---------- service: string - Name of the service to query: 'peaks' or 'ratings'. + Name of the service to query. Only ``'peaks'`` is served here; rating + tables come from :func:`get_ratings`, which uses a different + endpoint. ssl_check: bool, optional Whether to check SSL certificates. Default is True. **kwargs: optional diff --git a/dataretrieval/ogc/dates.py b/dataretrieval/ogc/dates.py index 1a5a4e095..471577a8f 100644 --- a/dataretrieval/ogc/dates.py +++ b/dataretrieval/ogc/dates.py @@ -111,7 +111,7 @@ def _format_api_dates( date: bool = False, *, name: str = "date input", - allow_duration: bool = True, + advertise_duration: bool = True, ) -> str | None: """ Formats date or datetime input(s) for use with an API. @@ -130,6 +130,18 @@ def _format_api_dates( date : bool, optional If True, uses only the date portion ("YYYY-MM-DD"). If False (default), returns full datetime in UTC ISO 8601 format ("YYYY-MM-DDTHH:MM:SSZ"). + name : str, optional + The caller's own spelling of this argument, used as the subject of + every message raised here. Defaults to a generic "date input"; pass + the real parameter name (``"time"``, ``"last_modified"``) so a caller + correcting the error edits an argument their getter actually accepts. + advertise_duration : bool, optional + Whether the "too many values" message offers an ISO 8601 duration as + an accepted single value. **Wording only -- this does not reject + durations.** A getter that refuses them (``get_ratings``, via + :func:`~dataretrieval.waterdata.ratings._validate_time_no_duration`) + enforces that itself and passes False here so the remedy does not + send a caller straight into its rejection. Returns ------- @@ -172,7 +184,7 @@ def _format_api_dates( f"{name} takes at most 2 values, got {len(items)}: {items!r}. " + ( "Pass one value for an instant or a duration ('2020-01-01', 'P7D'), " - if allow_duration + if advertise_duration else "Pass one value for an instant ('2020-01-01'), " ) + "or two for a closed interval ('2020-01-01', '2020-12-31')." diff --git a/dataretrieval/waterdata/ratings.py b/dataretrieval/waterdata/ratings.py index c64c50a3a..1cfb23730 100644 --- a/dataretrieval/waterdata/ratings.py +++ b/dataretrieval/waterdata/ratings.py @@ -177,7 +177,7 @@ def get_ratings( _validate_file_types(file_types) _validate_time_no_duration(time) time_str = ( - _format_api_dates(time, name="time", allow_duration=False) + _format_api_dates(time, name="time", advertise_duration=False) if time is not None else None ) diff --git a/dataretrieval/waterdata/reference.py b/dataretrieval/waterdata/reference.py index f5ea78f01..7e34b0d2c 100644 --- a/dataretrieval/waterdata/reference.py +++ b/dataretrieval/waterdata/reference.py @@ -41,9 +41,11 @@ def get_reference_table( One of the following options: "agency-codes", "altitude-datums", "aquifer-codes", "aquifer-types", "coordinate-accuracy-codes", "coordinate-datum-codes", "coordinate-method-codes", "counties", - "hydrologic-unit-codes", "medium-codes", "national-aquifer-codes", - "parameter-codes", "reliability-codes", "site-types", "states", - "statistic-codes", "topographic-codes", "time-zone-codes" + "countries", "hydrologic-unit-codes", "medium-codes", + "national-aquifer-codes", "parameter-codes", "reliability-codes", + "site-types", "states", "statistic-codes", "topographic-codes", + "time-zone-codes". ``METADATA_COLLECTIONS`` is the authoritative + list; a test pins this text against it. limit : int, optional The number of features returned in each page. The maximum allowable limit is 50000; the default (None) requests that maximum. Set a lower diff --git a/pyproject.toml b/pyproject.toml index eac78442b..65a95d50e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -203,9 +203,15 @@ skip_covered = true # rounded one -- 98.97% displays as "99%" and would otherwise pass a # ``fail_under = 99`` on rounding alone. precision = 2 -# The ratchet, set at the measured value. Raise it when coverage rises; never -# lower it to make a red build green -- lower it only deliberately, when a -# genuinely untestable path is added and excluding it would be dishonest. +# The ratchet, set just under the measured value. Raise it when coverage +# rises; never lower it to make a red build green -- lower it only +# deliberately, when a genuinely untestable path is added and excluding it +# would be dishonest. +# +# Verified identical (98.97%, 3683 stmts / 20 missing, 964 branches / 28 +# partial) on 3.12 and on 3.14, the version the blocking job runs -- branch +# arcs can differ between interpreters, so the threshold was checked against +# the one that enforces it rather than only the one it was written on. # # The remaining gap is concentrated in async fan-out internals # (transport/fanout, ogc/engine), the response-shaping dialect defaults diff --git a/tests/waterdata_test.py b/tests/waterdata_test.py index da17d968a..8e4043575 100644 --- a/tests/waterdata_test.py +++ b/tests/waterdata_test.py @@ -1508,3 +1508,18 @@ def test_get_reference_table_forwards_limit_as_a_query_arg(): fake.return_value = (pd.DataFrame(), mock.MagicMock(spec=[])) get_reference_table("agency-codes", limit=25) assert fake.call_args.kwargs["args"]["limit"] == 25 + + +def test_get_reference_table_docstring_lists_every_collection(): + """The docstring enumerates the vocabulary by hand, so it drifts the + moment a collection is added -- ``countries`` was served, accepted, and + absent from the docs. A reader who trusts the prose must not be told a + real collection does not exist. + """ + from typing import get_args + + from dataretrieval.waterdata.types import METADATA_COLLECTIONS + + doc = get_reference_table.__doc__ or "" + missing = [c for c in get_args(METADATA_COLLECTIONS) if f'"{c}"' not in doc] + assert not missing, f"collections served but undocumented: {missing}" diff --git a/tests/waterdata_utils_test.py b/tests/waterdata_utils_test.py index 507b2b933..d80385027 100644 --- a/tests/waterdata_utils_test.py +++ b/tests/waterdata_utils_test.py @@ -933,7 +933,7 @@ def test_the_duration_example_is_withheld_where_durations_are_rejected(): with pytest.raises(ValueError) as allowed: _format_api_dates(["a", "b", "c"], name="time") with pytest.raises(ValueError) as refused: - _format_api_dates(["a", "b", "c"], name="time", allow_duration=False) + _format_api_dates(["a", "b", "c"], name="time", advertise_duration=False) assert "'P7D'" in str(allowed.value) assert "'P7D'" not in str(refused.value) From 4e8dbb5473c8cf9e5b2e769f41c29ada185feb7c Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 21 Aug 2026 09:14:07 -0500 Subject: [PATCH 06/10] ci: enforce coverage across supported interpreters Move the coverage gate into the existing test matrix, keep Windows informational where POSIX-only tests skip, and exclude version-conditional branches so every supported Linux interpreter measures the same ratchet. --- .github/workflows/python-package.yml | 32 +++++++++++++--------------- pyproject.toml | 24 ++++++++++++++------- 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index 07f4f5dca..f4c138b7f 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -53,16 +53,6 @@ jobs: - name: Dependency-direction contracts # Rules and rationale live in .importlinter and the ADRs it cites. run: lint-imports - - name: Coverage ratchet - # Graded here rather than in the OS/Python matrix: several tests are - # POSIX-only (``skipif``), so a Windows or macOS run measures a - # genuinely smaller suite and would trip a shared threshold. The - # matrix still reports its own coverage; this is the one that blocks. - # Threshold and rationale live in [tool.coverage.report]. - run: | - pip install -e .[test,nldi] - coverage run -m pytest tests/ - coverage report - name: Complexity trend vs base # Advisory: reports which files moved and by how much, so a reviewer # can see direction rather than a pass/fail. Never fails the build -- @@ -155,16 +145,24 @@ jobs: run: | python -m pip install --upgrade pip pip install .[test,nldi] - - name: Test with pytest and report coverage + - name: Test with pytest # Pinned to bash on every OS. The default Windows shell is PowerShell, # which does not stop on a failing native command and takes the step's # exit code from the last one -- so a pytest failure was masked by the # coverage report that followed it, and the Windows matrix reported # success while tests were red. shell: bash - run: | - coverage run -m pytest tests/ - # --fail-under=0 disables the ratchet here on purpose: this matrix - # skips POSIX-only tests on Windows, so its number is informational. - # The gate that blocks runs once, on Linux, in the complexity job. - coverage report -m --fail-under=0 + run: coverage run -m pytest tests/ + - name: Coverage ratchet + # Blocks on every leg but Windows, where POSIX-only tests ``skipif`` + # out and the number measures a genuinely smaller suite. Phrased as + # "not Windows" rather than by naming one leg, so editing the matrix + # cannot leave the ratchet silently unenforced. The measurement is + # interpreter-independent -- see [tool.coverage.report]. + if: runner.os != 'Windows' + shell: bash + run: coverage report + - name: Coverage report (informational) + if: runner.os == 'Windows' + shell: bash + run: coverage report -m --fail-under=0 diff --git a/pyproject.toml b/pyproject.toml index 65a95d50e..3842cf309 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -208,10 +208,11 @@ precision = 2 # deliberately, when a genuinely untestable path is added and excluding it # would be dishonest. # -# Verified identical (98.97%, 3683 stmts / 20 missing, 964 branches / 28 -# partial) on 3.12 and on 3.14, the version the blocking job runs -- branch -# arcs can differ between interpreters, so the threshold was checked against -# the one that enforces it rather than only the one it was written on. +# Measured identically on 3.10, 3.13 and 3.14: 98.97%, 3695 statements / 20 +# missing, 956 branches / 28 partial. Every Linux leg of the test matrix +# enforces this threshold, so they have to agree -- 3.10 used to read 98.95% +# because the ``tomllib`` arm of a version-conditional import is dead there, +# which is why ``exclude_also`` drops version-conditional code below. # # The remaining gap is concentrated in async fan-out internals # (transport/fanout, ogc/engine), the response-shaping dialect defaults @@ -219,14 +220,21 @@ precision = 2 # each needs real scaffolding rather than a one-liner -- they are the next # rungs, not exclusions. fail_under = 98.9 +# Anchored to the start of a statement rather than matched anywhere on the +# line, so a pattern cannot also match its own words inside a comment or a +# string. Literal strings, because TOML rejects ``\s`` as an escape. exclude_also = [ # Type-checking-only imports never execute. - "if TYPE_CHECKING:", - "@overload", + '^\s*if TYPE_CHECKING:', + '^\s*@overload', # Environment fallbacks for a dependency or a package that is absent. The # suite installs the [test,nldi] extras, so these branches are unreachable # here by construction; reaching them means unimporting a module mid-run, # which tests the import system rather than this package. - "except ImportError", - "except PackageNotFoundError", + '^\s*except ImportError', + '^\s*except PackageNotFoundError', + # Version-conditional code: one arm is dead on every interpreter by + # construction, so measuring it makes the total depend on which version + # ran -- and the ratchet is enforced on every supported one. + '^\s*if sys\.version_info', ] From e01ed4dd6d44c9af32e9ebcdde1cd9cb55e235e3 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 21 Aug 2026 09:14:08 -0500 Subject: [PATCH 07/10] refactor(validation): complete shared check adoption Add the permissive any-of shape and the closed-vocabulary remedy extension needed by legacy NWIS and CQL callers, then replace the remaining hand-written recurring checks without changing their caller-facing intent. --- AGENTS.md | 10 ++-- dataretrieval/_validation.py | 91 +++++++++++++++++++++++++++++--- dataretrieval/nwdc.py | 15 ++---- dataretrieval/nwis.py | 95 +++++++++++++++++++--------------- dataretrieval/waterdata/cql.py | 13 +++-- tests/nwis_test.py | 2 +- tests/validation_test.py | 66 +++++++++++++++++++++++ tests/waterservices_test.py | 16 +++--- 8 files changed, 230 insertions(+), 78 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5dc5287ec..933f2d94b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -102,10 +102,14 @@ raise states the problem and then the move that fixes it, in that order. failure the remedy is whether to retry, and `transport.pagination. paginated_failure_message()` is the model: cause, then `To recover: …`. - Don't invent a phrasing for a check that recurs. `dataretrieval/_validation.py` - owns the wording for the four shared shapes — bad value in a closed vocabulary + owns the wording for the shared shapes — bad value in a closed vocabulary (`require_one_of`), missing argument (`require_argument`), incomplete group - (`require_together`), and conflicting arguments (`require_exactly_one`, - `reject_together`). Reach for one before hand-writing a message. + (`require_together`), no filter at all (`require_any_of`), and conflicting + arguments (`require_exactly_one`, `reject_together`). Reach for one before + hand-writing a message. Neither a service-specific pointer nor an exception + class is a reason to hand-write: every check takes a `remedy=` for the move it + cannot derive, and an `error=` for the class to raise (`nwis` passes + `TypeError`, which its query entry points raised long before this module). - `require_argument` returns the narrowed value, so use its result rather than re-testing for `None` to satisfy mypy — a second, unreachable message beside the first is how the two drift apart. diff --git a/dataretrieval/_validation.py b/dataretrieval/_validation.py index 3c082b436..77de59dbf 100644 --- a/dataretrieval/_validation.py +++ b/dataretrieval/_validation.py @@ -14,8 +14,16 @@ Three checks recur across the adapters, and each has a message shape here: a value outside a closed vocabulary (:func:`require_one_of`), an argument that is -missing (:func:`require_argument`, :func:`require_together`), and arguments that -cannot be combined (:func:`require_exactly_one`, :func:`reject_together`). +missing (:func:`require_argument`, :func:`require_together`, +:func:`require_any_of`), and arguments that cannot be combined +(:func:`require_exactly_one`, :func:`reject_together`). + +The exception class is the caller's, like the parameter name. These raise +``ValueError``, which is what the modern adapters raise; :mod:`~dataretrieval.nwis` +has answered a malformed query with ``TypeError`` since long before this module +existed, and a deprecated module cannot start raising a different class without +breaking the handlers written against it. ``error=TypeError`` lets it share the +wording without changing what callers catch. Every message states the problem and then the move that fixes it, in that order. Most callers of this package are programs -- a script, a pipeline stage, @@ -48,6 +56,8 @@ def require_one_of( *, name: str, context: str = "", + remedy: str = "", + error: type[Exception] = ValueError, ) -> None: """Raise ``ValueError`` unless *value* is one of *options*. @@ -68,6 +78,14 @@ def require_one_of( Optional qualifier for a vocabulary that depends on another argument, e.g. ``context="service 'wqp'"`` when the valid profiles differ per service. + remedy + A further move, for a vocabulary that is narrower than the service's: + how to reach what this function does not accept. Unlike the checks + below there is no remedy to override -- naming the options *is* the + message -- so this is added rather than substituted, and is omitted + when the options are the whole answer. + error + The exception class to raise; see the module docstring. Raises ------ @@ -81,9 +99,10 @@ def require_one_of( if value in options: return qualifier = f" for {context}" if context else "" - raise ValueError( + message = ( f"Invalid {name}: {value!r}{qualifier}. Valid options are: {_render(options)}." ) + raise error(f"{message} {remedy}" if remedy else message) def _render_names(names: Collection[str], *, conjunction: str = "and") -> str: @@ -119,6 +138,7 @@ def require_argument( *, context: str = "", remedy: str = "", + error: type[Exception] = ValueError, ) -> _T: """Return *value*, or raise ``ValueError`` if it was not supplied. @@ -142,6 +162,8 @@ def require_argument( remedy What to do instead, when the default ("pass a value") is not enough to act on -- typically the accepted forms or an example value. + error + The exception class to raise; see the module docstring. Returns ------- @@ -155,7 +177,7 @@ def require_argument( if value is not None: return value when = f" {context}" if context else "" - raise ValueError(f"{name} is required{when}. {remedy or f'Pass a {name} value.'}") + raise error(f"{name} is required{when}. {remedy or f'Pass a {name} value.'}") def require_together( @@ -163,6 +185,7 @@ def require_together( *, context: str = "", remedy: str = "", + error: type[Exception] = ValueError, ) -> None: """Raise ``ValueError`` unless *values* are all supplied or all omitted. @@ -182,6 +205,8 @@ def require_together( remedy Overrides the default remedy, which names the missing arguments to supply and the supplied ones to drop. + error + The exception class to raise; see the module docstring. Raises ------ @@ -195,17 +220,64 @@ def require_together( fix = remedy or ( f"Pass {_render_names(missing)}, or omit {_render_names(supplied)}." ) - raise ValueError( + raise error( f"{_render_names(values)} must be given together{where}. " f"Missing: {_render_names(missing)}. {fix}" ) +def require_any_of( + values: Mapping[str, object], + *, + context: str = "", + remedy: str = "", + error: type[Exception] = ValueError, +) -> None: + """Raise ``ValueError`` unless at least one of *values* was supplied. + + For a query that needs to be narrowed but does not care how -- the NWIS + major filters, where any one of five is enough for the service to answer. + The permissive sibling of :func:`require_exactly_one`: two of them is a + narrower query rather than a contradiction, so only none is an error. + + ``None`` counts as not supplied, per :func:`_supplied`, so a caller who + passes ``sites=None`` expecting it to be ignored is told the query has no + filter rather than having ``None`` spelled into the URL. + + Parameters + ---------- + values + Parameter name to supplied value, in the order the message should + list them. + context + What the group is for, when the parameter names do not say -- + ``context="to narrow the query"``. + remedy + Overrides the default remedy, which names the arguments to choose + among. + error + The exception class to raise; see the module docstring. + + Raises + ------ + ValueError + If none of *values* were supplied. + """ + supplied, _ = _supplied(values) + if supplied: + return + where = f" {context}" if context else "" + names = _render_names(values, conjunction="or") + fix = remedy or f"Pass one of {names}." + raise error(f"At least one of {names} is required{where}. {fix}") + + def require_exactly_one( values: Mapping[str, object], *, context: str = "", remedy: str = "", + error: type[Exception] = ValueError, ) -> None: """Raise ``ValueError`` unless exactly one of *values* was supplied. @@ -225,6 +297,8 @@ def require_exactly_one( remedy Overrides the default remedy, which is derived from which way the check failed. + error + The exception class to raise; see the module docstring. Raises ------ @@ -241,7 +315,7 @@ def require_exactly_one( else: fix = remedy or f"Pass one of {_render_names(values, conjunction='or')}." got = "none" - raise ValueError( + raise error( f"Provide exactly one of {_render_names(values, conjunction='or')}" f"{where}. Supplied: {got}. {fix}" ) @@ -252,6 +326,7 @@ def reject_together( *, context: str = "", remedy: str = "", + error: type[Exception] = ValueError, ) -> None: """Raise ``ValueError`` if more than one of *values* was supplied. @@ -270,6 +345,8 @@ def reject_together( remedy Overrides the default remedy, which names the supplied arguments to choose between. + error + The exception class to raise; see the module docstring. Raises ------ @@ -281,4 +358,4 @@ def reject_together( return why = f" -- {context}" if context else "" fix = remedy or f"Pass only one of {_render_names(supplied, conjunction='or')}." - raise ValueError(f"{_render_names(supplied)} cannot be combined{why}. {fix}") + raise error(f"{_render_names(supplied)} cannot be combined{why}. {fix}") diff --git a/dataretrieval/nwdc.py b/dataretrieval/nwdc.py index 60848972f..2141a8141 100644 --- a/dataretrieval/nwdc.py +++ b/dataretrieval/nwdc.py @@ -52,6 +52,7 @@ from dataretrieval import configuration as _configuration from dataretrieval._querying import _raise_for_status, to_str from dataretrieval._response_metadata import BaseMetadata +from dataretrieval._validation import require_exactly_one from dataretrieval.codes.states import to_state from dataretrieval.configuration import ( BaseConfiguration, @@ -300,17 +301,9 @@ def _resolve_locations( ``huc`` code's length selects its level (``huc2`` … ``huc12``). Returns one location string per value — the caller issues one request per location. """ - selected = { - name: value - for name, value in (("state", state), ("county", county), ("huc", huc)) - if value is not None - } - if len(selected) != 1: - raise ValueError( - "Specify exactly one of state, county, or huc " - f"(got: {', '.join(selected) or 'none'})." - ) - [(name, value)] = selected.items() + selectors = {"state": state, "county": county, "huc": huc} + require_exactly_one(selectors, context="as the query's location") + [(name, value)] = ((n, v) for n, v in selectors.items() if v is not None) locations = _LOCATION_BUILDERS[name](value) if not locations: raise ValueError( diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 56c552516..007896173 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -18,6 +18,11 @@ from dataretrieval._deprecation import REMOVALS, warn_deprecated from dataretrieval._response_metadata import BaseMetadata +from dataretrieval._validation import ( + require_any_of, + require_one_of, + require_together, +) from dataretrieval.exceptions import DataCurrencyWarning from dataretrieval.rdb import read_rdb @@ -37,6 +42,8 @@ ALLPARAMCODES_URL = "https://help.waterdata.usgs.gov/code/parameter_cd_query?" WATERSERVICES_SERVICES = ["dv", "iv", "site", "stat"] +# What ``get_record`` routes, which is wider than what ``query_waterdata`` +# reaches: 'ratings' is served by ``get_ratings`` from a different endpoint. WATERDATA_SERVICES = [ "peaks", "ratings", @@ -393,31 +400,36 @@ def query_waterdata( "se_latitude_va", ] - if not any(key in kwargs for key in major_params + bbox_params): - raise TypeError( - "Query must specify a major filter. Pass one of " - f"{', '.join(major_params)}, or all four bounding-box corners " - f"({', '.join(bbox_params)}) together with " + require_any_of( + {name: kwargs.get(name) for name in major_params + bbox_params}, + context="as a major filter", + remedy=( + f"Pass one of {' or '.join(major_params)}, or all four " + "bounding-box corners together with " "coordinate_format='decimal_degrees'." - ) - - elif any(key in kwargs for key in bbox_params) and not all( - key in kwargs for key in bbox_params - ): - absent = [key for key in bbox_params if key not in kwargs] - raise TypeError( - "A bounding box needs all four corners. Missing: " - f"{', '.join(absent)}. Pass them along with " - "coordinate_format='decimal_degrees', or drop the bounding box " - f"and filter with {' or '.join(major_params)} instead." - ) - - if service != "peaks": - raise TypeError( - f"Unrecognized service: {service!r}. query_waterdata serves " - "'peaks'. For rating tables call nwis.get_ratings(site=...), " - "which is served from a different endpoint." - ) + ), + error=TypeError, + ) + require_together( + {name: kwargs.get(name) for name in bbox_params}, + context="to describe a bounding box", + remedy=( + "Pass them along with coordinate_format='decimal_degrees', or " + f"drop the bounding box and filter with {' or '.join(major_params)} " + "instead." + ), + error=TypeError, + ) + require_one_of( + service, + ("peaks",), + name="service", + remedy=( + "For rating tables call nwis.get_ratings(site=...), which is " + "served from a different endpoint." + ), + error=TypeError, + ) url = WATERDATA_URL + service @@ -467,17 +479,12 @@ def query_waterservices( """ major_filters = ["sites", "stateCd", "bBox", "huc", "countyCd"] - if not any(key in kwargs for key in major_filters): - raise TypeError( - "Query must specify a major filter. Pass one of " - f"{', '.join(major_filters)}." - ) - - if service not in WATERSERVICES_SERVICES: - raise TypeError( - f"Unrecognized service: {service!r}. query_waterservices serves " - f"{', '.join(repr(name) for name in WATERSERVICES_SERVICES)}." - ) + require_any_of( + {name: kwargs.get(name) for name in major_filters}, + context="as a major filter", + error=TypeError, + ) + require_one_of(service, WATERSERVICES_SERVICES, name="service", error=TypeError) if "format" not in kwargs: kwargs["format"] = "rdb" @@ -976,14 +983,16 @@ def get_record( f"get_record. Use {defunct_replacements[service]} instead." ) - supported = WATERSERVICES_SERVICES + WATERDATA_SERVICES - if service not in supported: - raise TypeError( - f"Unrecognized service: {service!r}. get_record serves " - f"{', '.join(repr(name) for name in supported)}. New work should " - "use the dataretrieval.waterdata getters instead; NWIS is " - "deprecated." - ) + require_one_of( + service, + WATERSERVICES_SERVICES + WATERDATA_SERVICES, + name="service", + remedy=( + "New work should use the dataretrieval.waterdata getters instead; " + "NWIS is deprecated." + ), + error=TypeError, + ) if service == "iv": df, _ = get_iv( diff --git a/dataretrieval/waterdata/cql.py b/dataretrieval/waterdata/cql.py index 06829f267..357ada2e2 100644 --- a/dataretrieval/waterdata/cql.py +++ b/dataretrieval/waterdata/cql.py @@ -15,6 +15,7 @@ import pandas as pd from dataretrieval._deprecation import REMOVALS +from dataretrieval._validation import require_one_of from dataretrieval.waterdata.utils import ( _OUTPUT_ID_BY_COLLECTION, _accept_legacy_kwargs, @@ -147,14 +148,16 @@ def get_cql( ... ' "02070010%"]}', ... ) """ - if collection not in _OUTPUT_ID_BY_COLLECTION: - raise ValueError( - f"Invalid collection: {collection!r}. get_cql supports: " - f"{', '.join(repr(c) for c in sorted(_OUTPUT_ID_BY_COLLECTION))}. " + require_one_of( + collection, + sorted(_OUTPUT_ID_BY_COLLECTION), + name="collection", + remedy=( "The service serves more collections than these; for the full " "inventory call get_queryables(), which accepts " "any collection string." - ) + ), + ) # ``dict`` is the pythonic input — serialize on the way out. ``str`` is sent # verbatim so callers who already have a CQL2 doc (e.g. imported from a diff --git a/tests/nwis_test.py b/tests/nwis_test.py index 8d413d432..d43d4a781 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -341,7 +341,7 @@ def test_unrecognized_service_lists_the_ones_it_serves(self): with pytest.raises(TypeError) as excinfo: get_record(sites="01491000", service="nope") message = str(excinfo.value) - assert "Unrecognized service: 'nope'" in message + assert "Invalid service: 'nope'" in message assert "'iv'" in message and "'peaks'" in message assert "waterdata" in message diff --git a/tests/validation_test.py b/tests/validation_test.py index 689112aaa..fdc54957a 100644 --- a/tests/validation_test.py +++ b/tests/validation_test.py @@ -10,6 +10,7 @@ from dataretrieval._validation import ( reject_together, + require_any_of, require_argument, require_exactly_one, require_one_of, @@ -34,6 +35,23 @@ def test_context_qualifies_a_vocabulary_that_depends_on_another_argument(): require_one_of("x", ("a",), name="profile", context="service 'wqp'") +def test_remedy_adds_a_move_without_dropping_the_options(): + """A vocabulary narrower than the service's needs both halves. + + ``get_cql`` accepts the collections it can shape; the service serves more, + so the message has to name what this function takes *and* how to reach the + rest. Unlike the group checks there is no derived remedy to replace here -- + naming the options is the message. + """ + with pytest.raises(ValueError) as excinfo: + require_one_of( + "hourly", ("daily",), name="collection", remedy="Call get_queryables." + ) + message = str(excinfo.value) + assert "Valid options are: 'daily'." in message + assert message.endswith("Call get_queryables.") + + def test_a_string_vocabulary_is_refused(): """``str`` is a Collection, so passing one type-checks -- and then ``in`` silently degrades from membership to a substring test, accepting any @@ -87,6 +105,35 @@ def test_reports_every_missing_member_of_a_larger_group(self): require_together({"a": 1, "b": None, "c": None}) +class TestRequireAnyOf: + def test_accepts_one_supplied(self): + require_any_of({"sites": "01491000", "stateCd": None}) + + def test_accepts_more_than_one(self): + """Two filters narrow the query; only none of them is an error.""" + require_any_of({"sites": "01491000", "stateCd": "WI"}) + + def test_message_names_every_argument_that_would_have_served(self): + with pytest.raises(ValueError) as excinfo: + require_any_of({"sites": None, "stateCd": None, "huc": None}) + message = str(excinfo.value) + assert "At least one of sites, stateCd or huc is required" in message + assert "Pass one of sites, stateCd or huc." in message + + def test_context_says_what_the_group_is_for(self): + with pytest.raises(ValueError, match="as a major filter"): + require_any_of({"sites": None}, context="as a major filter") + + def test_remedy_replaces_the_default(self): + with pytest.raises(ValueError, match="Pass all four corners."): + require_any_of({"sites": None}, remedy="Pass all four corners.") + + def test_an_explicit_none_is_not_a_filter(self): + """``sites=None`` counted as supplied is how ``None`` reaches a URL.""" + with pytest.raises(ValueError): + require_any_of({"sites": None}) + + class TestRequireExactlyOne: def test_accepts_exactly_one(self): require_exactly_one({"comid": 1, "feature_source": None}) @@ -129,3 +176,22 @@ def test_context_explains_why_they_conflict(self): reject_together( {"lat": 1.0, "comid": 2}, context="they name different origins" ) + + +@pytest.mark.parametrize( + "check", + [ + lambda: require_one_of("x", ("a",), name="service", error=TypeError), + lambda: require_argument("service", None, error=TypeError), + lambda: require_together({"a": 1, "b": None}, error=TypeError), + lambda: require_any_of({"a": None}, error=TypeError), + lambda: require_exactly_one({"a": None, "b": None}, error=TypeError), + lambda: reject_together({"a": 1, "b": 2}, error=TypeError), + ], +) +def test_every_check_raises_the_callers_exception_class(check): + """``nwis`` has answered a malformed query with ``TypeError`` since long + before this module existed. Sharing the wording must not change what a + caller catches, or adopting it here would be a breaking change.""" + with pytest.raises(TypeError): + check() diff --git a/tests/waterservices_test.py b/tests/waterservices_test.py index 2674a0f55..ba63ee834 100644 --- a/tests/waterservices_test.py +++ b/tests/waterservices_test.py @@ -33,14 +33,14 @@ def test_query_waterdata_validation(): with pytest.raises(TypeError) as type_error: query_waterdata(service="pmcodes", format="rdb") message = str(type_error.value) - assert "Query must specify a major filter" in message + assert "is required as a major filter" in message assert "site_no, stateCd" in message assert "nw_longitude_va" in message with pytest.raises(TypeError) as type_error: query_waterdata(service=None, site_no="sites") message = str(type_error.value) - assert "Unrecognized service: None" in message + assert "Invalid service: None" in message # 'ratings' was advertised here but is not an NwisWeb program: the URL it # built returned an HTML error page, not data. assert "'peaks'" in message @@ -49,9 +49,9 @@ def test_query_waterdata_validation(): with pytest.raises(TypeError) as type_error: query_waterdata(service="pmcodes", nw_longitude_va="something") message = str(type_error.value) - assert "bounding box needs all four corners" in message + assert "must be given together to describe a bounding box" in message # The three corners actually absent, so the caller knows what to add. - assert "nw_latitude_va, se_longitude_va, se_latitude_va" in message + assert "nw_latitude_va, se_longitude_va and se_latitude_va" in message def test_query_waterservices_validation(): @@ -59,13 +59,13 @@ def test_query_waterservices_validation(): with pytest.raises(TypeError) as type_error: query_waterservices(service="dv", format="rdb") message = str(type_error.value) - assert "Query must specify a major filter" in message - assert "sites, stateCd, bBox, huc, countyCd" in message + assert "is required as a major filter" in message + assert "sites, stateCd, bBox, huc or countyCd" in message with pytest.raises(TypeError) as type_error: query_waterservices(service=None, sites="sites") message = str(type_error.value) - assert "Unrecognized service: None" in message + assert "Invalid service: None" in message assert "'dv', 'iv', 'site', 'stat'" in message @@ -94,7 +94,7 @@ def test_get_record_validation(): with pytest.raises(TypeError) as type_error: get_record(sites=["01491000"], service="not_a_service") message = str(type_error.value) - assert "Unrecognized service: 'not_a_service'" in message + assert "Invalid service: 'not_a_service'" in message assert "'dv', 'iv', 'site', 'stat', 'peaks', 'ratings'" in message From a839f9a8301760974316d0a95c8385ab176baae3 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 21 Aug 2026 09:14:10 -0500 Subject: [PATCH 08/10] docs: record behavior changes and remove stale commentary Record the public behavior changes in NEWS and remove comments that merely restated measured values or nearby code while retaining rationale and constraints. --- .github/workflows/python-package.yml | 7 +-- NEWS.md | 2 + dataretrieval/_response_metadata.py | 1 - dataretrieval/_validation.py | 66 +++++++++++----------------- dataretrieval/codes/states.py | 5 +-- dataretrieval/nldi.py | 10 ++--- dataretrieval/waterdata/reference.py | 5 +-- pyproject.toml | 45 +++++-------------- tests/nldi_test.py | 7 +-- tests/nwdc_test.py | 7 +-- tests/validation_test.py | 13 ++---- 11 files changed, 53 insertions(+), 115 deletions(-) diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index f4c138b7f..807532c89 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -154,11 +154,8 @@ jobs: shell: bash run: coverage run -m pytest tests/ - name: Coverage ratchet - # Blocks on every leg but Windows, where POSIX-only tests ``skipif`` - # out and the number measures a genuinely smaller suite. Phrased as - # "not Windows" rather than by naming one leg, so editing the matrix - # cannot leave the ratchet silently unenforced. The measurement is - # interpreter-independent -- see [tool.coverage.report]. + # Windows skips POSIX-only tests, so its number measures a smaller + # suite. Negative condition: a matrix edit cannot silently unenforce it. if: runner.os != 'Windows' shell: bash run: coverage report diff --git a/NEWS.md b/NEWS.md index 137ba5bf2..10d317232 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,5 @@ +**08/20/2026:** Argument checks now share one vocabulary, and every rejection names a move the caller can execute. `dataretrieval._validation` owns the message shape for each check that recurs across the adapters -- a value outside a closed vocabulary (`require_one_of`), a missing argument (`require_argument`, `require_together`), a query with no filter at all (`require_any_of`), and arguments that conflict (`require_exactly_one`, `reject_together`) -- so a new check cannot invent its own phrasing, which is how `get_reference_table` came to tell callers who passed a bad `collection` that their *code service* was invalid. Each check takes the caller's own spelling of the parameter, a remedy for the move it cannot derive, and the exception class to raise, so `nwis` keeps answering a malformed query with `TypeError` while sharing the wording. **Behavior change:** the text of those rejections moves with them -- `"Unrecognized service: 'x'. get_record serves …"` is now `"Invalid service: 'x'. Valid options are: …"`, and the major-filter and bounding-box complaints in `query_waterdata` / `query_waterservices` are rendered in the shared form. The exception classes are unchanged; code matching on the old strings is not. **Bug fix:** a `None` passed as a major filter (`query_waterservices(service='dv', sites=None)`) counted as a filter and reached the service as an empty `sites=`; `None` now means not supplied, and the call is refused with the filters that would have served. **Bug fix:** four messages told callers to do something that raised again or named a parameter their getter does not accept -- `nldi.get_features(comid=…, feature_id=…)` said to supply the missing half of the feature pair, and the corrected call then failed on the conflict with `comid`; `nwdc` answered `state=[]` by saying exactly one of `state`, `county` or `huc` must be given, when exactly one was; `codes.states.apply_state` offered NGWMN callers a `state_name` / `state_code` parameter no NGWMN getter accepts; and `BaseMetadata` pointed NGWMN and NWDC callers at a Water Data getter. **Bug fix:** `nldi.get_features(navigation_mode=…)` without a `data_source` spelled `None` into the URL path and returned an empty frame from a 200; it now raises and names the source to pass. **Behavior change:** `waterdata.get_nearest_continuous` appends `time` and `monitoring_location_id` to an explicit `properties` list rather than honoring it verbatim -- omitting either silently collapsed every monitoring location into one row per target, a wrong answer rather than an error -- so a caller who passed `properties=['time', 'value']` now gets a third column. **Behavior change:** `nwis.query_waterdata` serves `'peaks'` only; the `'ratings'` URL it used to build was never an NwisWeb program and answered with an HTML error page. `nwis.get_record(service='ratings')` is unaffected -- it routes to `get_ratings`, which is served from a different endpoint. New: `waterdata.get_cql(..., max_rows=N)` caps the total rows a CQL query returns. + **08/13/2026:** Warning categories now say what they mean. Two advisories about *upstream data* were emitted as `DeprecationWarning` and as an uncategorized `warnings.warn` respectively; both are now `DataCurrencyWarning` (a `UserWarning` subclass, exported as `dataretrieval.DataCurrencyWarning`). **Behavior change:** WQP's legacy-WQX notice moves from `DeprecationWarning` to `DataCurrencyWarning`. Because `legacy=True` is the default on every WQP getter and the notice is unconditional, a downstream project running `-W error::DeprecationWarning` previously could not call any WQP getter with default arguments; it now can. The flip side is visibility — `DeprecationWarning` is silent by default outside `__main__`, so this notice will now print to stderr for library and notebook callers who never saw it. Silence it with `warnings.filterwarnings("ignore", category=dataretrieval.DataCurrencyWarning)`, or set `legacy=False` where a WQX3.0 profile exists. The NWIS qw-endpoint retirement notice gains the same category (it previously had none, arriving as a bare `UserWarning` that deprecation filters ignored). `DeprecationWarning` now means only that a name in this package is going away, always with a replacement and, where published, a removal date; those horizons are declared once in `dataretrieval._deprecation.REMOVALS` rather than spelled at each call site. **08/11/2026:** Settings resolve through a layered chain instead of the environment alone, and a *configuration profile* is a named set of settings for **one adapter**. The new `dataretrieval.configuration` module resolves every setting in one order, highest first: a configuration passed to an active `dataretrieval.configure(...)` block, a profile that block selected, the setting's `API_USGS_*` environment variable, the adapter's `[]` table in `~/.dataretrieval/config.toml` (or `DATARETRIEVAL_CONFIG`), the file's top-level keys, the adapter's own built-in preference, then the package default. Precedence applies **per setting**, so a file that sets only `concurrency` leaves an environment `API_USGS_PAT` in effect, and a `[ngwmn]` table still inherits every top-level key it does not name. `configure()` takes configuration objects positionally, at most one per adapter and nothing else: `configure(Configuration(api_key=vault.read("usgs/pat")), WaterdataConfiguration.load("bulk"), NgwmnConfiguration(concurrency=4))`. The adapter a configuration targets is a property of its class, so a caller never restates it, and each adapter owns its class in the module that *reads* those settings (`waterdata.WaterdataConfiguration`, `ngwmn.NgwmnConfiguration`, `nwdc.NwdcConfiguration`, `wqp.WqpConfiguration`, `nldi.NldiConfiguration`, `streamstats.StreamstatsConfiguration`) — an adapter accepts only the settings it reads, so `[streamstats] parallel_chunks = 8` is an error rather than a line that quietly does nothing. The block is delivered through a `ContextVar`, so a credential set inside it cannot leak across threads or asyncio tasks, which is what makes it safe for a server or notebook handling several users' keys and is the thing assigning to `os.environ` could never do (issue #352). The file gains named profiles beside each adapter's default profile: `[waterdata]` is always in effect, `[waterdata.bulk]` only when a caller selects it with `WaterdataConfiguration.load("bulk")`, and a selected profile still inherits the default profile and the package-wide keys per setting. A profile named in code outranks the setting's environment variable — the one place the ladder inverts the environment-above-file rule, because losing a deliberate selection to a stale shell export is what a caller would file a bug about. An adapter's configuration may also carry a `base_url`, which redirects that adapter's requests for the duration of the block — a staging instance, a mirror, a recording proxy — and no other adapter's; for Water Data one value moves the OGC collections, the Samples database, the statistics service and the STAC catalog together. It is settable in a `configure()` block only: a `base_url` key in the file and an exported `API_USGS_BASE_URL` each raise rather than being read, since a redirect a config file or a shell profile can set is one no reader of the script can see. The API key does not follow a redirect — it is scoped to the single host that honors it — and is deliberately not per-adapter: it authenticates to the gateway fronting a host, and Water Data and NGWMN share that host, one key, and one hourly quota. `dataretrieval.show_configuration()` reports each setting's effective value and where it came from, naming the profile behind each value (`configure() block [waterdata.bulk]` rather than a bare block), listing the profiles a file defines whether or not this run selected any, and naming any adapter this process has not imported rather than omitting it — without ever printing the key. One parser per setting owns its grammar, so a value means the same thing whichever source wrote it. **Breaking change:** `RetryPolicy.from_env()` is now `RetryPolicy.from_configuration()` and resolves through the whole chain rather than the environment alone. **Behavior change:** a credential-shaped keyword passed to a getter's `**kwargs` query passthrough — Water Data's `**queryables` and every WQP getter's search filters — now raises `TypeError` naming `configure(Configuration(api_key=...))` instead of putting a secret in a URL that clients, proxies and logs retain. The names refused are `api_key=`, `token=`, `x_api_key=`, `password=`, `auth=`, `pat=` and similar spellings; a filter the server actually defines is unaffected. **Bug fix:** `API_USGS_STALL_TIMEOUT` was read straight from `os.environ`, so it could not be set by a `configure()` block or the config file and never appeared in `show_configuration()`; it now resolves through the chain like every other setting. Rationale in ADRs 0009, 0010 and 0011; terms in `CONTEXT.md`. diff --git a/dataretrieval/_response_metadata.py b/dataretrieval/_response_metadata.py index 5c39fa807..a4cdbf6b9 100644 --- a/dataretrieval/_response_metadata.py +++ b/dataretrieval/_response_metadata.py @@ -53,7 +53,6 @@ def __init__(self, response: httpx.Response) -> None: # # disclaimer seems to be only part of importWaterML1 # self.disclaimer = None - # Set by the ``nwis`` / ``wqp`` metadata subclasses only. @property def site_info(self) -> Any: raise NotImplementedError( diff --git a/dataretrieval/_validation.py b/dataretrieval/_validation.py index 77de59dbf..879c01582 100644 --- a/dataretrieval/_validation.py +++ b/dataretrieval/_validation.py @@ -1,36 +1,25 @@ """Argument checks shared by every adapter. -Rejecting a value that is not in a closed vocabulary is the one validation -every adapter does, and it was written eleven times: eight message phrasings -for one concept, so each new check was a coin flip on wording. That is how -:func:`~dataretrieval.waterdata.get_reference_table` came to tell callers who -passed a bad ``collection`` that their *code service* was invalid -- the check -was copied from :mod:`~dataretrieval.waterdata.samples`, message and local -variable name included, and the noun was never changed. - -This module owns the wording so a new check cannot invent its own. It is a -leaf with no first-party imports: the vocabularies it validates against live -with the adapters that define them, and only the rejection is shared. - -Three checks recur across the adapters, and each has a message shape here: a -value outside a closed vocabulary (:func:`require_one_of`), an argument that is -missing (:func:`require_argument`, :func:`require_together`, -:func:`require_any_of`), and arguments that cannot be combined -(:func:`require_exactly_one`, :func:`reject_together`). - -The exception class is the caller's, like the parameter name. These raise -``ValueError``, which is what the modern adapters raise; :mod:`~dataretrieval.nwis` -has answered a malformed query with ``TypeError`` since long before this module -existed, and a deprecated module cannot start raising a different class without -breaking the handlers written against it. ``error=TypeError`` lets it share the -wording without changing what callers catch. - -Every message states the problem and then the move that fixes it, in that -order. Most callers of this package are programs -- a script, a pipeline stage, -an agent -- and a program cannot infer from "Service not recognized" which -services exist. Naming the remedy is what lets the caller correct itself -without a human reading the source, so a check that cannot name one is a check -whose message is not finished. +Rejecting a value outside a closed vocabulary is the one validation every +adapter does, and it was written eleven times in eight phrasings -- which is +how :func:`~dataretrieval.waterdata.get_reference_table` came to tell callers +who passed a bad ``collection`` that their *code service* was invalid. This +module owns the wording so a new check cannot invent its own. It is a leaf +with no first-party imports: the vocabularies live with the adapters that +define them, and only the rejection is shared. + +Three shapes recur: a value outside a closed vocabulary +(:func:`require_one_of`), a missing argument (:func:`require_argument`, +:func:`require_together`, :func:`require_any_of`), and arguments that cannot be +combined (:func:`require_exactly_one`, :func:`reject_together`). The exception +class is the caller's, like the parameter name: ``error=TypeError`` lets +:mod:`~dataretrieval.nwis` share the wording without changing what its callers +already catch. + +Every message states the problem and then the move that fixes it. Most callers +here are programs, and a program cannot infer from "Service not recognized" +which services exist -- so a check that cannot name a remedy is a check whose +message is not finished. """ from __future__ import annotations @@ -79,11 +68,10 @@ def require_one_of( e.g. ``context="service 'wqp'"`` when the valid profiles differ per service. remedy - A further move, for a vocabulary that is narrower than the service's: - how to reach what this function does not accept. Unlike the checks - below there is no remedy to override -- naming the options *is* the - message -- so this is added rather than substituted, and is omitted - when the options are the whole answer. + A further move, for a vocabulary narrower than the service's: how to + reach what this function does not accept. Added rather than + substituted -- unlike the checks below, there is no derived remedy + here, since naming the options *is* the message. error The exception class to raise; see the module docstring. @@ -239,10 +227,8 @@ def require_any_of( major filters, where any one of five is enough for the service to answer. The permissive sibling of :func:`require_exactly_one`: two of them is a narrower query rather than a contradiction, so only none is an error. - - ``None`` counts as not supplied, per :func:`_supplied`, so a caller who - passes ``sites=None`` expecting it to be ignored is told the query has no - filter rather than having ``None`` spelled into the URL. + ``None`` counts as not supplied, so ``sites=None`` is refused rather than + reaching the URL. Parameters ---------- diff --git a/dataretrieval/codes/states.py b/dataretrieval/codes/states.py index 648c2155e..552ed2284 100644 --- a/dataretrieval/codes/states.py +++ b/dataretrieval/codes/states.py @@ -240,9 +240,8 @@ def apply_state( try: local_vars[into] = to_state(state, to) except ValueError as err: - # ``into`` leads when it is also a rejected spelling (it is the - # queryable this endpoint filters on), but it is never offered on its - # own strength -- only ``reject`` proves the getter accepts the name. + # Only ``reject`` proves the getter accepts a spelling; ``into`` is the + # wire queryable, so it leads only when it appears there too. offered = dict.fromkeys(n for n in (into, *reject) if n in reject) native = " or ".join(offered) if not native: diff --git a/dataretrieval/nldi.py b/dataretrieval/nldi.py index 7f6210583..53872030a 100644 --- a/dataretrieval/nldi.py +++ b/dataretrieval/nldi.py @@ -53,13 +53,12 @@ _AVAILABLE_DATA_SOURCES = None _CRS = "EPSG:4326" _VALID_NAVIGATION_MODES = ("UM", "DM", "UT", "DD") -#: The modes rendered for a message. Built from the tuple above rather than -#: written beside it, so a mode added there cannot go unmentioned here. +#: Built from the tuple above, so a mode added there cannot go unmentioned. _NAVIGATION_MODES_HINT = ( f"Pass one of {', '.join(repr(mode) for mode in _VALID_NAVIGATION_MODES)}." ) -#: The two ways to name an origin. Shared by the conflict check and the -#: nothing-supplied check so the same pair of ways forward is offered either way. +#: Shared by the conflict check and the nothing-supplied check, so both +#: offer the same ways forward. _ORIGIN_HINT = ( "Navigate from a comid, e.g. comid=13294314, or from a " "feature_source/feature_id pair -- not both" @@ -490,9 +489,6 @@ def _search_basin(feature_source: str | None, feature_id: str | None) -> dict[st "Pass both, e.g. feature_source='WQP', feature_id='USGS-01031500'; " "a basin has no other origin." ) - # ``require_together`` reports a half-supplied pair, naming both sides at - # once; it permits the pair being absent entirely, which the two checks - # below reject. Together they cover every way the origin can be incomplete. require_together( {"feature_source": feature_source, "feature_id": feature_id}, context="for find='basin'", diff --git a/dataretrieval/waterdata/reference.py b/dataretrieval/waterdata/reference.py index 7e34b0d2c..bda54bdde 100644 --- a/dataretrieval/waterdata/reference.py +++ b/dataretrieval/waterdata/reference.py @@ -97,9 +97,8 @@ def get_reference_table( require_one_of(collection, get_args(METADATA_COLLECTIONS), name="collection") # Give the ID column the collection name, singularized and underscored. - # ``removesuffix`` rather than an ``endswith`` branch: every collection in - # the vocabulary is plural today, so the non-plural arm was unreachable, - # and this stays correct if a singular one is ever added. + # ``removesuffix`` rather than an ``endswith`` branch, whose non-plural arm + # was unreachable and would stay correct if a singular collection appeared. if collection in ("counties", "countries"): output_id = collection[:-3] + "y" # county / country else: diff --git a/pyproject.toml b/pyproject.toml index 3842cf309..4da15c3d3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -186,55 +186,32 @@ markers = [ [tool.coverage.run] source = ["dataretrieval"] -# Branch coverage, because most of what this package gets wrong is a branch -# rather than a line: a service-dispatch arm that routes to the wrong getter, -# an error path that never fires, a fallback that silently becomes the norm. +# Most of what this package gets wrong is a branch rather than a line. branch = true omit = [ - # Written by setuptools_scm at build time. Not ours, and absent from a - # source checkout until something builds. + # Written by setuptools_scm at build time; absent from a source checkout. "dataretrieval/_version.py", ] [tool.coverage.report] show_missing = true skip_covered = true -# Two decimals so the gate compares against the real number rather than a -# rounded one -- 98.97% displays as "99%" and would otherwise pass a -# ``fail_under = 99`` on rounding alone. +# 98.97% displays as "99%", which would pass a ``fail_under = 99`` on rounding. precision = 2 -# The ratchet, set just under the measured value. Raise it when coverage -# rises; never lower it to make a red build green -- lower it only -# deliberately, when a genuinely untestable path is added and excluding it -# would be dishonest. -# -# Measured identically on 3.10, 3.13 and 3.14: 98.97%, 3695 statements / 20 -# missing, 956 branches / 28 partial. Every Linux leg of the test matrix -# enforces this threshold, so they have to agree -- 3.10 used to read 98.95% -# because the ``tomllib`` arm of a version-conditional import is dead there, -# which is why ``exclude_also`` drops version-conditional code below. -# -# The remaining gap is concentrated in async fan-out internals -# (transport/fanout, ogc/engine), the response-shaping dialect defaults -# (ogc/shaping), and a handful of partial branches. Those are reachable, but -# each needs real scaffolding rather than a one-liner -- they are the next -# rungs, not exclusions. +# A ratchet: raise it as coverage rises. Lower it only when a genuinely +# untestable path is added and excluding it would be dishonest. fail_under = 98.9 -# Anchored to the start of a statement rather than matched anywhere on the -# line, so a pattern cannot also match its own words inside a comment or a -# string. Literal strings, because TOML rejects ``\s`` as an escape. +# Anchored to the statement, so a pattern cannot match its own words in a +# comment or a string. Literal strings, because TOML rejects ``\s``. exclude_also = [ # Type-checking-only imports never execute. '^\s*if TYPE_CHECKING:', '^\s*@overload', - # Environment fallbacks for a dependency or a package that is absent. The - # suite installs the [test,nldi] extras, so these branches are unreachable - # here by construction; reaching them means unimporting a module mid-run, - # which tests the import system rather than this package. + # The suite installs the [test,nldi] extras, so an absent-dependency fallback + # is unreachable here by construction. '^\s*except ImportError', '^\s*except PackageNotFoundError', - # Version-conditional code: one arm is dead on every interpreter by - # construction, so measuring it makes the total depend on which version - # ran -- and the ratchet is enforced on every supported one. + # One arm is dead on every interpreter, which would otherwise make the total + # depend on which version measured it. '^\s*if sys\.version_info', ] diff --git a/tests/nldi_test.py b/tests/nldi_test.py index 948f82de1..405a90e02 100644 --- a/tests/nldi_test.py +++ b/tests/nldi_test.py @@ -440,12 +440,7 @@ def test_search_for_basin_names_the_missing_half(kwargs, problem): ], ) def test_half_a_feature_pair_beside_a_comid_is_reported_as_a_conflict(half, supplied): - """Completing the pair would only raise the origin conflict next. - - The pair check fired first, so the caller was told to supply the missing - half, and the corrected call then failed on ``comid`` -- two round trips - for one mistake. - """ + """Completing the pair would only raise the origin conflict next.""" with pytest.raises(ValueError) as excinfo: _validate_feature_source_comid(comid=13294314, **supplied) message = str(excinfo.value) diff --git a/tests/nwdc_test.py b/tests/nwdc_test.py index b92ad58ca..5b7504902 100644 --- a/tests/nwdc_test.py +++ b/tests/nwdc_test.py @@ -410,12 +410,7 @@ def test_resolve_locations_empty_list_rejected(): def test_empty_selector_is_not_reported_as_the_wrong_selector_count(): - """One selector was given; the fault is its value, not how many there are. - - The message used to close with "exactly one of state, county, or huc must - be given", sending a caller who passed ``state=[]`` to change ``county`` - or ``huc`` instead of filling in ``state``. - """ + """One selector was given; the fault is its value, not how many there are.""" with pytest.raises(ValueError) as excinfo: _resolve_locations([], None, None) message = str(excinfo.value) diff --git a/tests/validation_test.py b/tests/validation_test.py index fdc54957a..f27c34d9e 100644 --- a/tests/validation_test.py +++ b/tests/validation_test.py @@ -36,13 +36,8 @@ def test_context_qualifies_a_vocabulary_that_depends_on_another_argument(): def test_remedy_adds_a_move_without_dropping_the_options(): - """A vocabulary narrower than the service's needs both halves. - - ``get_cql`` accepts the collections it can shape; the service serves more, - so the message has to name what this function takes *and* how to reach the - rest. Unlike the group checks there is no derived remedy to replace here -- - naming the options is the message. - """ + """A vocabulary narrower than the service's needs both halves: what this + function takes, and how to reach the rest.""" with pytest.raises(ValueError) as excinfo: require_one_of( "hourly", ("daily",), name="collection", remedy="Call get_queryables." @@ -190,8 +185,6 @@ def test_context_explains_why_they_conflict(self): ], ) def test_every_check_raises_the_callers_exception_class(check): - """``nwis`` has answered a malformed query with ``TypeError`` since long - before this module existed. Sharing the wording must not change what a - caller catches, or adopting it here would be a breaking change.""" + """Sharing the wording must not change what a caller already catches.""" with pytest.raises(TypeError): check() From 60a0c6de8642de7c5a2c77be9a97a3d10be02d3c Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 21 Aug 2026 09:14:13 -0500 Subject: [PATCH 09/10] feat(states): support US territories Add the five inhabited US territories to state normalization, allow Water Data and NGWMN queries the upstream services already support, and make endpoint-specific remedies use arguments their public getter accepts. --- NEWS.md | 2 ++ dataretrieval/codes/states.py | 34 +++++++++++------- dataretrieval/ngwmn.py | 4 +-- dataretrieval/nwis.py | 8 +++-- tests/utils_test.py | 68 ++++++++++++++++++++++------------- 5 files changed, 74 insertions(+), 42 deletions(-) diff --git a/NEWS.md b/NEWS.md index 10d317232..3f3c0281b 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,5 @@ +**08/20/2026:** The `state` filter now accepts the five US territories. `dataretrieval.codes.states` held the 50 states and DC, so `ngwmn.get_sites(state='Puerto Rico')`, `waterdata.get_monitoring_locations(state_name='Puerto Rico')` via the unified `state`, and `nwdc.get_wateruse(state='PR')` were refused locally -- while all three services carry the data (NGWMN answers with 36 Puerto Rico monitoring locations, the Water Data monitoring-locations collection returns Puerto Rico sites, and legacy NWIS lists 1,148 stream sites for `stateCd=PR`). American Samoa, Guam, the Northern Mariana Islands, Puerto Rico and the US Virgin Islands are now in both code tables under their real ANSI/FIPS codes, so every encoding resolves: `'Puerto Rico'`, `'PR'`, `'72'` and `'US:72'` all normalize alike. **Behavior change:** a territory that used to raise `ValueError` now produces a request. A value the table genuinely does not hold still fails fast. + **08/20/2026:** Argument checks now share one vocabulary, and every rejection names a move the caller can execute. `dataretrieval._validation` owns the message shape for each check that recurs across the adapters -- a value outside a closed vocabulary (`require_one_of`), a missing argument (`require_argument`, `require_together`), a query with no filter at all (`require_any_of`), and arguments that conflict (`require_exactly_one`, `reject_together`) -- so a new check cannot invent its own phrasing, which is how `get_reference_table` came to tell callers who passed a bad `collection` that their *code service* was invalid. Each check takes the caller's own spelling of the parameter, a remedy for the move it cannot derive, and the exception class to raise, so `nwis` keeps answering a malformed query with `TypeError` while sharing the wording. **Behavior change:** the text of those rejections moves with them -- `"Unrecognized service: 'x'. get_record serves …"` is now `"Invalid service: 'x'. Valid options are: …"`, and the major-filter and bounding-box complaints in `query_waterdata` / `query_waterservices` are rendered in the shared form. The exception classes are unchanged; code matching on the old strings is not. **Bug fix:** a `None` passed as a major filter (`query_waterservices(service='dv', sites=None)`) counted as a filter and reached the service as an empty `sites=`; `None` now means not supplied, and the call is refused with the filters that would have served. **Bug fix:** four messages told callers to do something that raised again or named a parameter their getter does not accept -- `nldi.get_features(comid=…, feature_id=…)` said to supply the missing half of the feature pair, and the corrected call then failed on the conflict with `comid`; `nwdc` answered `state=[]` by saying exactly one of `state`, `county` or `huc` must be given, when exactly one was; `codes.states.apply_state` offered NGWMN callers a `state_name` / `state_code` parameter no NGWMN getter accepts; and `BaseMetadata` pointed NGWMN and NWDC callers at a Water Data getter. **Bug fix:** `nldi.get_features(navigation_mode=…)` without a `data_source` spelled `None` into the URL path and returned an empty frame from a 200; it now raises and names the source to pass. **Behavior change:** `waterdata.get_nearest_continuous` appends `time` and `monitoring_location_id` to an explicit `properties` list rather than honoring it verbatim -- omitting either silently collapsed every monitoring location into one row per target, a wrong answer rather than an error -- so a caller who passed `properties=['time', 'value']` now gets a third column. **Behavior change:** `nwis.query_waterdata` serves `'peaks'` only; the `'ratings'` URL it used to build was never an NwisWeb program and answered with an HTML error page. `nwis.get_record(service='ratings')` is unaffected -- it routes to `get_ratings`, which is served from a different endpoint. New: `waterdata.get_cql(..., max_rows=N)` caps the total rows a CQL query returns. **08/13/2026:** Warning categories now say what they mean. Two advisories about *upstream data* were emitted as `DeprecationWarning` and as an uncategorized `warnings.warn` respectively; both are now `DataCurrencyWarning` (a `UserWarning` subclass, exported as `dataretrieval.DataCurrencyWarning`). **Behavior change:** WQP's legacy-WQX notice moves from `DeprecationWarning` to `DataCurrencyWarning`. Because `legacy=True` is the default on every WQP getter and the notice is unconditional, a downstream project running `-W error::DeprecationWarning` previously could not call any WQP getter with default arguments; it now can. The flip side is visibility — `DeprecationWarning` is silent by default outside `__main__`, so this notice will now print to stderr for library and notebook callers who never saw it. Silence it with `warnings.filterwarnings("ignore", category=dataretrieval.DataCurrencyWarning)`, or set `legacy=False` where a WQX3.0 profile exists. The NWIS qw-endpoint retirement notice gains the same category (it previously had none, arriving as a bare `UserWarning` that deprecation filters ignored). `DeprecationWarning` now means only that a name in this package is going away, always with a replacement and, where published, a removal date; those horizons are declared once in `dataretrieval._deprecation.REMOVALS` rather than spelled at each call site. diff --git a/dataretrieval/codes/states.py b/dataretrieval/codes/states.py index 552ed2284..1599d1122 100644 --- a/dataretrieval/codes/states.py +++ b/dataretrieval/codes/states.py @@ -6,7 +6,7 @@ a state identifier -- a full name, postal code, or two-digit / ``US:``-prefixed FIPS code (or an iterable of them) -- to a chosen representation. An unrecognized value raises ``ValueError``. Coverage is the -50 states plus the District of Columbia. +50 states, the District of Columbia, and the five US territories. """ from __future__ import annotations @@ -66,6 +66,11 @@ "West Virginia": "wv", "Wisconsin": "wi", "Wyoming": "wy", + "American Samoa": "as", + "Guam": "gu", + "Northern Mariana Islands": "mp", + "Puerto Rico": "pr", + "US Virgin Islands": "vi", } fips_codes = { @@ -120,6 +125,11 @@ "West Virginia": "54", "Wisconsin": "55", "Wyoming": "56", + "American Samoa": "60", + "Guam": "66", + "Northern Mariana Islands": "69", + "Puerto Rico": "72", + "US Virgin Islands": "78", } # Reverse lookups (built once): postal code -> name, FIPS code -> name, and a @@ -150,9 +160,10 @@ def to_state( * ``"fips"`` -> two-digit ANSI/FIPS code, e.g. ``"55"`` * ``"fips_us"`` -> ``"US:"`` + FIPS code, e.g. ``"US:55"`` - Coverage is the 50 states plus the District of Columbia. A ``value`` that - isn't a recognized state in one of those encodings raises ``ValueError`` - (so a typo fails fast rather than silently matching nothing). + Coverage is the 50 states, DC, and the five US territories, each under its + real ANSI/FIPS code. A ``value`` that isn't recognized in one of those + encodings raises ``ValueError``, so a typo fails fast rather than + silently matching nothing. """ if isinstance(value, str): return _to_state_one(value, to) @@ -175,12 +186,9 @@ def _to_state_one(value: str | int, to: str) -> str: if name is None: raise ValueError( - f"{value!r} is not a recognized US state or the District of " - f'Columbia. Provide a full name ("Wisconsin"), a two-letter postal ' - f'code ("WI"), or a two-digit ANSI/FIPS code ("55"). Coverage is ' - f"the 50 states and DC only -- a US territory (Puerto Rico, Guam, " - f"US Virgin Islands, American Samoa, Northern Mariana Islands) " - f"has no entry in this table." + f"{value!r} is not a recognized US state, district, or " + f'territory. Provide a full name ("Wisconsin"), a two-letter ' + f'postal code ("WI"), or a two-digit ANSI/FIPS code ("55").' ) return _format_state(name, to) @@ -215,8 +223,8 @@ def apply_state( ``state`` alongside any of them raises ``ValueError``. Returns the (mutated) ``local_vars``. - An unrecognized ``state`` -- a US territory, say -- is re-raised naming the - parameters in ``reject``, and only those. They are the endpoint's own state + An unrecognized ``state`` is re-raised naming the parameters in ``reject``, + and only those. They are the endpoint's own state parameters *as the caller spells them*: the mutual-exclusion guard below is proof the getter accepts them as keyword arguments. ``into`` is deliberately not offered, because it is a wire queryable that need not exist on the @@ -248,6 +256,6 @@ def apply_state( raise raise ValueError( f"{err} Pass {native} instead -- they take the values the API " - f"itself uses, territories included." + f"itself uses, unnormalized." ) from err return local_vars diff --git a/dataretrieval/ngwmn.py b/dataretrieval/ngwmn.py index f6ac2da4f..ff68486c9 100644 --- a/dataretrieval/ngwmn.py +++ b/dataretrieval/ngwmn.py @@ -182,7 +182,7 @@ def get_sites( state : str or iterable of str, optional State filter. Accepts a full name (``"Wisconsin"``), a two-letter postal code (``"WI"``), or a two-digit ANSI/FIPS code (``"55"``). - The 50 states and DC only; a US territory is rejected. + The 50 states, DC, and the five US territories. county_name : str or iterable of str, optional County name filter. aquifer_name, site_type, aquifer_type_code : str or iterable, optional @@ -413,7 +413,7 @@ def get_providers( state : str or iterable of str, optional State filter. Accepts a full name (``"Wisconsin"``), a two-letter postal code (``"WI"``), or a two-digit ANSI/FIPS code (``"55"``). - The 50 states and DC only; a US territory is rejected. Only one + The 50 states, DC, and the five US territories. Only one state at a time — a multi-value state filter returns no records for this collection. agency_code : str or iterable of str, optional diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 007896173..55fc5035f 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -404,7 +404,7 @@ def query_waterdata( {name: kwargs.get(name) for name in major_params + bbox_params}, context="as a major filter", remedy=( - f"Pass one of {' or '.join(major_params)}, or all four " + "Pass one, e.g. site_no='01491000' or stateCd='WI', or all four " "bounding-box corners together with " "coordinate_format='decimal_degrees'." ), @@ -425,8 +425,9 @@ def query_waterdata( ("peaks",), name="service", remedy=( - "For rating tables call nwis.get_ratings(site=...), which is " - "served from a different endpoint." + "Rating tables come from waterdata.get_ratings(" + "monitoring_location_id='USGS-01646500'), served from a different " + "endpoint and keyed by the AGENCY-ID form of the site number." ), error=TypeError, ) @@ -482,6 +483,7 @@ def query_waterservices( require_any_of( {name: kwargs.get(name) for name in major_filters}, context="as a major filter", + remedy=("Pass one, e.g. sites='01491000', stateCd='WI', or countyCd='55025'."), error=TypeError, ) require_one_of(service, WATERSERVICES_SERVICES, name="service", error=TypeError) diff --git a/tests/utils_test.py b/tests/utils_test.py index ca74c7a34..e5616ea26 100644 --- a/tests/utils_test.py +++ b/tests/utils_test.py @@ -473,15 +473,50 @@ def test_the_table_names_no_endpoint_parameter_of_its_own(self): from dataretrieval.codes.states import to_state with pytest.raises(ValueError) as excinfo: - to_state("Puerto Rico") + to_state("Atlantis") message = str(excinfo.value) - assert "no entry in this table" in message assert "state_name" not in message assert "state_code" not in message -class TestApplyStateTerritories: - """A territory has no row in the table, so the remedy is the endpoint's own +class TestTerritories: + """The five territories are real ANSI/FIPS entities, and every service this + package reaches carries data for them.""" + + @pytest.mark.parametrize( + ("value", "name", "postal", "fips"), + [ + ("Puerto Rico", "Puerto Rico", "PR", "72"), + ("PR", "Puerto Rico", "PR", "72"), + ("72", "Puerto Rico", "PR", "72"), + ("US:72", "Puerto Rico", "PR", "72"), + ("Guam", "Guam", "GU", "66"), + ("VI", "US Virgin Islands", "VI", "78"), + ("60", "American Samoa", "AS", "60"), + ("MP", "Northern Mariana Islands", "MP", "69"), + ], + ) + def test_every_encoding_resolves(self, value, name, postal, fips): + from dataretrieval.codes.states import to_state + + assert to_state(value, "name") == name + assert to_state(value, "postal") == postal + assert to_state(value, "fips") == fips + + def test_the_ngwmn_shim_routes_a_territory_to_each_queryable(self): + """``sites`` filters on ``state_name``, ``providers`` on ``state``.""" + from dataretrieval.codes.states import apply_state + + assert apply_state({"state": "Puerto Rico"}, to="name", into="state_name") == { + "state_name": "Puerto Rico" + } + assert apply_state({"state": "Puerto Rico"}, to="postal", into="state") == { + "state": "PR" + } + + +class TestApplyStateUnrecognized: + """The remedy for a value the table does not hold is the endpoint's own state queryable -- and *which* one differs per endpoint, so the message has to be built where those names are known.""" @@ -490,7 +525,7 @@ def test_the_remedy_names_this_endpoints_native_parameters(self): with pytest.raises(ValueError) as excinfo: apply_state( - {"state": "Puerto Rico"}, + {"state": "Atlantis"}, to="name", into="state_name", reject=("state_code", "state_name"), @@ -500,30 +535,15 @@ def test_the_remedy_names_this_endpoints_native_parameters(self): assert "Pass state_name or state_code instead" in message def test_an_endpoint_with_no_alternative_offers_none(self): - """NGWMN's getters expose only the unified ``state``: ``sites`` - filters on the ``state_name`` queryable but does not accept it as an - argument, and ``providers``' queryable *is* ``state``. Appending a + """NGWMN's getters expose only the unified ``state``, so appending a remedy from ``into`` sent a caller to ``get_sites(state_name=...)`` - (``TypeError``) or straight back into this same error. - """ + (``TypeError``) or straight back into this same error.""" from dataretrieval.codes.states import apply_state for into, to in (("state", "postal"), ("state_name", "name")): with pytest.raises(ValueError) as excinfo: - apply_state({"state": "Guam"}, to=to, into=into) - message = str(excinfo.value) - assert "no entry in this table" in message - assert "instead" not in message - - def test_the_ngwmn_getters_reject_a_territory_without_misdirecting(self): - """End to end: whatever the message names must be an argument the - getter the caller actually called accepts.""" - from dataretrieval import ngwmn - - for getter in (ngwmn.get_sites, ngwmn.get_providers): - with pytest.raises(ValueError) as excinfo: - getter(state="Puerto Rico") - assert "state_name" not in str(excinfo.value) + apply_state({"state": "Atlantis"}, to=to, into=into) + assert "instead" not in str(excinfo.value) def test_retrying_get_maps_invalid_url(monkeypatch): From 5d7ff503220332438cb27b698725926f8c794c3e Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 21 Aug 2026 09:14:15 -0500 Subject: [PATCH 10/10] refactor(validation): simplify checks and retire legacy exceptions Consolidate shared message rendering and context handling, finish adoption in NLDI, NWIS, states, and ratings, return validated selections instead of re-deriving them, and standardize malformed argument values on ValueError while improving the NWIS ratings replacement guidance. --- AGENTS.md | 15 +-- NEWS.md | 2 +- dataretrieval/_validation.py | 148 ++++++++++++++------------- dataretrieval/codes/states.py | 29 +++--- dataretrieval/nldi.py | 71 +++++-------- dataretrieval/nwdc.py | 7 +- dataretrieval/nwis.py | 56 +++++----- dataretrieval/ogc/dates.py | 21 ++-- dataretrieval/waterdata/ratings.py | 9 +- dataretrieval/waterdata/reference.py | 2 +- tests/nldi_test.py | 2 +- tests/nwis_test.py | 2 +- tests/utils_test.py | 2 +- tests/validation_test.py | 24 +---- tests/waterdata_utils_test.py | 10 +- tests/waterservices_test.py | 30 +++--- 16 files changed, 199 insertions(+), 231 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 933f2d94b..9b0bf9b1e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -106,13 +106,14 @@ raise states the problem and then the move that fixes it, in that order. (`require_one_of`), missing argument (`require_argument`), incomplete group (`require_together`), no filter at all (`require_any_of`), and conflicting arguments (`require_exactly_one`, `reject_together`). Reach for one before - hand-writing a message. Neither a service-specific pointer nor an exception - class is a reason to hand-write: every check takes a `remedy=` for the move it - cannot derive, and an `error=` for the class to raise (`nwis` passes - `TypeError`, which its query entry points raised long before this module). -- `require_argument` returns the narrowed value, so use its result rather than - re-testing for `None` to satisfy mypy — a second, unreachable message beside - the first is how the two drift apart. + hand-writing a message. A service-specific pointer is not a reason to + hand-write: every check takes a `remedy=` for the move it cannot derive. Every + check raises `ValueError` -- one class for a bad argument value, so a caller + catches by shape rather than by which module rejected it. +- `require_argument` returns the narrowed value and `require_exactly_one` the + winning `(name, value)` pair, so use their results rather than re-testing for + `None` to satisfy mypy — a second, unreachable message beside the first is + how the two drift apart. - **Paste the remedy back before trusting it.** Whatever a message names must be a real parameter of the function the *caller* called — not a private helper's local, not a prose label — and following it literally must produce a working diff --git a/NEWS.md b/NEWS.md index 3f3c0281b..c9cb7177f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,6 @@ **08/20/2026:** The `state` filter now accepts the five US territories. `dataretrieval.codes.states` held the 50 states and DC, so `ngwmn.get_sites(state='Puerto Rico')`, `waterdata.get_monitoring_locations(state_name='Puerto Rico')` via the unified `state`, and `nwdc.get_wateruse(state='PR')` were refused locally -- while all three services carry the data (NGWMN answers with 36 Puerto Rico monitoring locations, the Water Data monitoring-locations collection returns Puerto Rico sites, and legacy NWIS lists 1,148 stream sites for `stateCd=PR`). American Samoa, Guam, the Northern Mariana Islands, Puerto Rico and the US Virgin Islands are now in both code tables under their real ANSI/FIPS codes, so every encoding resolves: `'Puerto Rico'`, `'PR'`, `'72'` and `'US:72'` all normalize alike. **Behavior change:** a territory that used to raise `ValueError` now produces a request. A value the table genuinely does not hold still fails fast. -**08/20/2026:** Argument checks now share one vocabulary, and every rejection names a move the caller can execute. `dataretrieval._validation` owns the message shape for each check that recurs across the adapters -- a value outside a closed vocabulary (`require_one_of`), a missing argument (`require_argument`, `require_together`), a query with no filter at all (`require_any_of`), and arguments that conflict (`require_exactly_one`, `reject_together`) -- so a new check cannot invent its own phrasing, which is how `get_reference_table` came to tell callers who passed a bad `collection` that their *code service* was invalid. Each check takes the caller's own spelling of the parameter, a remedy for the move it cannot derive, and the exception class to raise, so `nwis` keeps answering a malformed query with `TypeError` while sharing the wording. **Behavior change:** the text of those rejections moves with them -- `"Unrecognized service: 'x'. get_record serves …"` is now `"Invalid service: 'x'. Valid options are: …"`, and the major-filter and bounding-box complaints in `query_waterdata` / `query_waterservices` are rendered in the shared form. The exception classes are unchanged; code matching on the old strings is not. **Bug fix:** a `None` passed as a major filter (`query_waterservices(service='dv', sites=None)`) counted as a filter and reached the service as an empty `sites=`; `None` now means not supplied, and the call is refused with the filters that would have served. **Bug fix:** four messages told callers to do something that raised again or named a parameter their getter does not accept -- `nldi.get_features(comid=…, feature_id=…)` said to supply the missing half of the feature pair, and the corrected call then failed on the conflict with `comid`; `nwdc` answered `state=[]` by saying exactly one of `state`, `county` or `huc` must be given, when exactly one was; `codes.states.apply_state` offered NGWMN callers a `state_name` / `state_code` parameter no NGWMN getter accepts; and `BaseMetadata` pointed NGWMN and NWDC callers at a Water Data getter. **Bug fix:** `nldi.get_features(navigation_mode=…)` without a `data_source` spelled `None` into the URL path and returned an empty frame from a 200; it now raises and names the source to pass. **Behavior change:** `waterdata.get_nearest_continuous` appends `time` and `monitoring_location_id` to an explicit `properties` list rather than honoring it verbatim -- omitting either silently collapsed every monitoring location into one row per target, a wrong answer rather than an error -- so a caller who passed `properties=['time', 'value']` now gets a third column. **Behavior change:** `nwis.query_waterdata` serves `'peaks'` only; the `'ratings'` URL it used to build was never an NwisWeb program and answered with an HTML error page. `nwis.get_record(service='ratings')` is unaffected -- it routes to `get_ratings`, which is served from a different endpoint. New: `waterdata.get_cql(..., max_rows=N)` caps the total rows a CQL query returns. +**08/20/2026:** Argument checks now share one vocabulary, and every rejection names a move the caller can execute. `dataretrieval._validation` owns the message shape for each check that recurs across the adapters -- a value outside a closed vocabulary (`require_one_of`), a missing argument (`require_argument`, `require_together`), a query with no filter at all (`require_any_of`), and arguments that conflict (`require_exactly_one`, `reject_together`) -- so a new check cannot invent its own phrasing, which is how `get_reference_table` came to tell callers who passed a bad `collection` that their *code service* was invalid. Each check takes the caller's own spelling of the parameter and a remedy for the move it cannot derive, and every check raises `ValueError` -- one class for a bad argument value. **Behavior change:** the text of those rejections moves with them -- `"Unrecognized service: 'x'. get_record serves …"` is now `"Invalid service: 'x'. Valid options are: …"`, and the major-filter and bounding-box complaints in `query_waterdata` / `query_waterservices` are rendered in the shared form. **Behavior change:** the deprecated `nwis` query entry points (`query_waterdata`, `query_waterservices`, `get_record`) now answer a missing major filter, an incomplete bounding box, or an unknown service with `ValueError` rather than their historic `TypeError` -- `TypeError` remains for a genuinely mistyped argument, such as a non-string `sites`. Code catching `TypeError` there, or matching on the old strings, must update. **Bug fix:** a `None` passed as a major filter (`query_waterservices(service='dv', sites=None)`) counted as a filter and reached the service as an empty `sites=`; `None` now means not supplied, and the call is refused with the filters that would have served. **Bug fix:** four messages told callers to do something that raised again or named a parameter their getter does not accept -- `nldi.get_features(comid=…, feature_id=…)` said to supply the missing half of the feature pair, and the corrected call then failed on the conflict with `comid`; `nwdc` answered `state=[]` by saying exactly one of `state`, `county` or `huc` must be given, when exactly one was; `codes.states.apply_state` offered NGWMN callers a `state_name` / `state_code` parameter no NGWMN getter accepts; and `BaseMetadata` pointed NGWMN and NWDC callers at a Water Data getter. **Bug fix:** `nldi.get_features(navigation_mode=…)` without a `data_source` spelled `None` into the URL path and returned an empty frame from a 200; it now raises and names the source to pass. **Behavior change:** `waterdata.get_nearest_continuous` appends `time` and `monitoring_location_id` to an explicit `properties` list rather than honoring it verbatim -- omitting either silently collapsed every monitoring location into one row per target, a wrong answer rather than an error -- so a caller who passed `properties=['time', 'value']` now gets a third column. **Behavior change:** `nwis.query_waterdata` serves `'peaks'` only; the `'ratings'` URL it used to build was never an NwisWeb program and answered with an HTML error page. `nwis.get_record(service='ratings')` is unaffected -- it routes to `get_ratings`, which is served from a different endpoint. New: `waterdata.get_cql(..., max_rows=N)` caps the total rows a CQL query returns. **08/13/2026:** Warning categories now say what they mean. Two advisories about *upstream data* were emitted as `DeprecationWarning` and as an uncategorized `warnings.warn` respectively; both are now `DataCurrencyWarning` (a `UserWarning` subclass, exported as `dataretrieval.DataCurrencyWarning`). **Behavior change:** WQP's legacy-WQX notice moves from `DeprecationWarning` to `DataCurrencyWarning`. Because `legacy=True` is the default on every WQP getter and the notice is unconditional, a downstream project running `-W error::DeprecationWarning` previously could not call any WQP getter with default arguments; it now can. The flip side is visibility — `DeprecationWarning` is silent by default outside `__main__`, so this notice will now print to stderr for library and notebook callers who never saw it. Silence it with `warnings.filterwarnings("ignore", category=dataretrieval.DataCurrencyWarning)`, or set `legacy=False` where a WQX3.0 profile exists. The NWIS qw-endpoint retirement notice gains the same category (it previously had none, arriving as a bare `UserWarning` that deprecation filters ignored). `DeprecationWarning` now means only that a name in this package is going away, always with a replacement and, where published, a removal date; those horizons are declared once in `dataretrieval._deprecation.REMOVALS` rather than spelled at each call site. diff --git a/dataretrieval/_validation.py b/dataretrieval/_validation.py index 879c01582..ff07fa25b 100644 --- a/dataretrieval/_validation.py +++ b/dataretrieval/_validation.py @@ -11,10 +11,11 @@ Three shapes recur: a value outside a closed vocabulary (:func:`require_one_of`), a missing argument (:func:`require_argument`, :func:`require_together`, :func:`require_any_of`), and arguments that cannot be -combined (:func:`require_exactly_one`, :func:`reject_together`). The exception -class is the caller's, like the parameter name: ``error=TypeError`` lets -:mod:`~dataretrieval.nwis` share the wording without changing what its callers -already catch. +combined (:func:`require_exactly_one`, :func:`reject_together`). Every check +raises ``ValueError``: each is a complaint about an argument's *value*, and one +class means a caller's ``except`` needs no inventory of which check fired. +(Deprecated :mod:`~dataretrieval.nwis` historically raised ``TypeError`` from +its query entry points; it now raises ``ValueError`` like everything else.) Every message states the problem and then the move that fixes it. Most callers here are programs, and a program cannot infer from "Service not recognized" @@ -30,15 +31,57 @@ class is the caller's, like the parameter name: ``error=TypeError`` lets _T = TypeVar("_T") -def _render(options: Collection[object]) -> str: +def render_options(options: Collection[object]) -> str: """Format *options* for a message: ``'a', 'b', 'c'``. Renders the values rather than their container so ``dict_keys([...])`` and a bare tuple read the same to a caller, who never sees the container. + Shared with adapters that mention a vocabulary outside a rejection -- a + hint constant, a message appended to a service's own error -- so every + list of values reads the same everywhere. """ return ", ".join(repr(option) for option in options) +def _render_names(names: Collection[str], *, conjunction: str = "and") -> str: + """Format parameter *names* for a message: ``a``, ``a and b``, ``a, b and c``. + + Bare, not quoted: these are the caller's own parameter names, so they read + as identifiers to paste back into the call rather than as data values -- + which is what :func:`render_options` is for. + """ + listed = list(names) + if len(listed) <= 1: + return "".join(listed) + return f"{', '.join(listed[:-1])} {conjunction} {listed[-1]}" + + +def _qualify(context: str, *, prefix: str = " ") -> str: + """Return *context* ready to splice into a message, or nothing. + + Every check appends its caller's ``context`` the same way; owning the + splice here keeps a new check from inventing a fifth local spelling of + ``f" {context}" if context else ""``. + """ + return f"{prefix}{context}" if context else "" + + +def _supplied(values: Mapping[str, object]) -> tuple[list[str], list[str]]: + """Split *values* into the names that were supplied and those that were not. + + ``None`` is the package's "not supplied" marker throughout the public + signatures, so it is the one this module tests for. A caller whose sentinel + differs -- an empty string that should count as missing -- normalizes to + ``None`` before calling, rather than this module guessing which falsy values + were meant. + """ + supplied: list[str] = [] + missing: list[str] = [] + for name, value in values.items(): + (missing if value is None else supplied).append(name) + return supplied, missing + + def require_one_of( value: object, options: Collection[object], @@ -46,7 +89,6 @@ def require_one_of( name: str, context: str = "", remedy: str = "", - error: type[Exception] = ValueError, ) -> None: """Raise ``ValueError`` unless *value* is one of *options*. @@ -72,8 +114,6 @@ def require_one_of( reach what this function does not accept. Added rather than substituted -- unlike the checks below, there is no derived remedy here, since naming the options *is* the message. - error - The exception class to raise; see the module docstring. Raises ------ @@ -86,38 +126,12 @@ def require_one_of( raise TypeError(f"options must be a collection of values, not {options!r}") if value in options: return - qualifier = f" for {context}" if context else "" + qualifier = _qualify(context, prefix=" for ") message = ( - f"Invalid {name}: {value!r}{qualifier}. Valid options are: {_render(options)}." + f"Invalid {name}: {value!r}{qualifier}. " + f"Valid options are: {render_options(options)}." ) - raise error(f"{message} {remedy}" if remedy else message) - - -def _render_names(names: Collection[str], *, conjunction: str = "and") -> str: - """Format parameter *names* for a message: ``a``, ``a and b``, ``a, b and c``. - - Bare, not quoted: these are the caller's own parameter names, so they read - as identifiers to paste back into the call rather than as data values -- - which is what :func:`_render` is for. - """ - listed = list(names) - if len(listed) <= 1: - return "".join(listed) - return f"{', '.join(listed[:-1])} {conjunction} {listed[-1]}" - - -def _supplied(values: Mapping[str, object]) -> tuple[list[str], list[str]]: - """Split *values* into the names that were supplied and those that were not. - - ``None`` is the package's "not supplied" marker throughout the public - signatures, so it is the one this module tests for. A caller whose sentinel - differs -- an empty string that should count as missing -- normalizes to - ``None`` before calling, rather than this module guessing which falsy values - were meant. - """ - supplied = [name for name, value in values.items() if value is not None] - missing = [name for name, value in values.items() if value is None] - return supplied, missing + raise ValueError(f"{message} {remedy}" if remedy else message) def require_argument( @@ -126,7 +140,6 @@ def require_argument( *, context: str = "", remedy: str = "", - error: type[Exception] = ValueError, ) -> _T: """Return *value*, or raise ``ValueError`` if it was not supplied. @@ -150,8 +163,6 @@ def require_argument( remedy What to do instead, when the default ("pass a value") is not enough to act on -- typically the accepted forms or an example value. - error - The exception class to raise; see the module docstring. Returns ------- @@ -164,8 +175,8 @@ def require_argument( """ if value is not None: return value - when = f" {context}" if context else "" - raise error(f"{name} is required{when}. {remedy or f'Pass a {name} value.'}") + when = _qualify(context) + raise ValueError(f"{name} is required{when}. {remedy or f'Pass a {name} value.'}") def require_together( @@ -173,7 +184,6 @@ def require_together( *, context: str = "", remedy: str = "", - error: type[Exception] = ValueError, ) -> None: """Raise ``ValueError`` unless *values* are all supplied or all omitted. @@ -193,8 +203,6 @@ def require_together( remedy Overrides the default remedy, which names the missing arguments to supply and the supplied ones to drop. - error - The exception class to raise; see the module docstring. Raises ------ @@ -204,11 +212,11 @@ def require_together( supplied, missing = _supplied(values) if not supplied or not missing: return - where = f" {context}" if context else "" + where = _qualify(context) fix = remedy or ( f"Pass {_render_names(missing)}, or omit {_render_names(supplied)}." ) - raise error( + raise ValueError( f"{_render_names(values)} must be given together{where}. " f"Missing: {_render_names(missing)}. {fix}" ) @@ -219,7 +227,6 @@ def require_any_of( *, context: str = "", remedy: str = "", - error: type[Exception] = ValueError, ) -> None: """Raise ``ValueError`` unless at least one of *values* was supplied. @@ -241,8 +248,6 @@ def require_any_of( remedy Overrides the default remedy, which names the arguments to choose among. - error - The exception class to raise; see the module docstring. Raises ------ @@ -252,25 +257,28 @@ def require_any_of( supplied, _ = _supplied(values) if supplied: return - where = f" {context}" if context else "" + where = _qualify(context) names = _render_names(values, conjunction="or") fix = remedy or f"Pass one of {names}." - raise error(f"At least one of {names} is required{where}. {fix}") + raise ValueError(f"At least one of {names} is required{where}. {fix}") def require_exactly_one( - values: Mapping[str, object], + values: Mapping[str, _T | None], *, context: str = "", remedy: str = "", - error: type[Exception] = ValueError, -) -> None: - """Raise ``ValueError`` unless exactly one of *values* was supplied. +) -> tuple[str, _T]: + """Return the one supplied ``(name, value)``, or raise ``ValueError``. For a choice between alternatives that are each sufficient on their own -- the origin of an NLDI navigation, the location selector of an NWDC query. Both failure directions are reported by the same check because they have - the same fix from opposite sides: supply one, or drop the rest. + the same fix from opposite sides: supply one, or drop the rest. The + winning pair is returned for the same reason :func:`require_argument` + returns its value: the caller's next move is to dispatch on it, and + re-deriving it beside the call restates the invariant this check just + proved. Parameters ---------- @@ -283,25 +291,28 @@ def require_exactly_one( remedy Overrides the default remedy, which is derived from which way the check failed. - error - The exception class to raise; see the module docstring. + + Returns + ------- + The supplied ``(name, value)`` pair, its value narrowed to non-``None``. Raises ------ ValueError If none of *values* were supplied, or more than one was. """ - supplied, _ = _supplied(values) - if len(supplied) == 1: - return - where = f" {context}" if context else "" + selected = [(name, value) for name, value in values.items() if value is not None] + if len(selected) == 1: + return selected[0] + supplied = [name for name, _ in selected] + where = _qualify(context) if supplied: fix = remedy or f"Drop all but one of {_render_names(supplied)}." got = _render_names(supplied) else: fix = remedy or f"Pass one of {_render_names(values, conjunction='or')}." got = "none" - raise error( + raise ValueError( f"Provide exactly one of {_render_names(values, conjunction='or')}" f"{where}. Supplied: {got}. {fix}" ) @@ -312,7 +323,6 @@ def reject_together( *, context: str = "", remedy: str = "", - error: type[Exception] = ValueError, ) -> None: """Raise ``ValueError`` if more than one of *values* was supplied. @@ -331,8 +341,6 @@ def reject_together( remedy Overrides the default remedy, which names the supplied arguments to choose between. - error - The exception class to raise; see the module docstring. Raises ------ @@ -342,6 +350,6 @@ def reject_together( supplied, _ = _supplied(values) if len(supplied) < 2: return - why = f" -- {context}" if context else "" + why = _qualify(context, prefix=" -- ") fix = remedy or f"Pass only one of {_render_names(supplied, conjunction='or')}." - raise error(f"{_render_names(supplied)} cannot be combined{why}. {fix}") + raise ValueError(f"{_render_names(supplied)} cannot be combined{why}. {fix}") diff --git a/dataretrieval/codes/states.py b/dataretrieval/codes/states.py index 1599d1122..55bce3d0e 100644 --- a/dataretrieval/codes/states.py +++ b/dataretrieval/codes/states.py @@ -14,6 +14,8 @@ from collections.abc import Iterable from typing import Any +from dataretrieval._validation import reject_together, require_one_of + state_codes = { "Alabama": "al", "Alaska": "ak", @@ -196,15 +198,14 @@ def _to_state_one(value: str | int, to: str) -> str: def _format_state(name: str, to: str) -> str: """Render a canonical state *name* in the ``to`` representation.""" + require_one_of(to, ("name", "postal", "fips", "fips_us"), name="to") if to == "name": return name if to == "postal": return state_codes[name].upper() if to == "fips": return fips_codes[name] - if to == "fips_us": - return f"US:{fips_codes[name]}" - raise ValueError(f"to must be 'name', 'postal', 'fips', or 'fips_us'; got {to!r}") + return f"US:{fips_codes[name]}" def apply_state( @@ -237,25 +238,21 @@ def apply_state( state = local_vars.pop("state", None) if state is None: return local_vars - # Name only the parameters actually supplied: a caller told to choose - # between `state` and an argument it never passed cannot act on the message. - conflicting = " or ".join(p for p in reject if local_vars.get(p) is not None) - if conflicting: - raise ValueError( - f"state cannot be combined with {conflicting} -- they filter on " - f"the same thing. Pass state, or {conflicting}, not both." - ) + reject_together( + {"state": state, **{p: local_vars.get(p) for p in reject}}, + context="they filter on the same thing", + ) try: local_vars[into] = to_state(state, to) except ValueError as err: + if not reject: + # No native spelling of the getter's own to offer instead. + raise # Only ``reject`` proves the getter accepts a spelling; ``into`` is the # wire queryable, so it leads only when it appears there too. offered = dict.fromkeys(n for n in (into, *reject) if n in reject) - native = " or ".join(offered) - if not native: - raise raise ValueError( - f"{err} Pass {native} instead -- they take the values the API " - f"itself uses, unnormalized." + f"{err} Pass {' or '.join(offered)} instead -- they take the " + f"values the API itself uses, unnormalized." ) from err return local_vars diff --git a/dataretrieval/nldi.py b/dataretrieval/nldi.py index 53872030a..c46afd931 100644 --- a/dataretrieval/nldi.py +++ b/dataretrieval/nldi.py @@ -18,6 +18,7 @@ from dataretrieval._querying import _query_with_retry from dataretrieval._validation import ( reject_together, + render_options, require_argument, require_exactly_one, require_one_of, @@ -54,15 +55,15 @@ _CRS = "EPSG:4326" _VALID_NAVIGATION_MODES = ("UM", "DM", "UT", "DD") #: Built from the tuple above, so a mode added there cannot go unmentioned. -_NAVIGATION_MODES_HINT = ( - f"Pass one of {', '.join(repr(mode) for mode in _VALID_NAVIGATION_MODES)}." -) +_NAVIGATION_MODES_HINT = f"Pass one of {render_options(_VALID_NAVIGATION_MODES)}." #: Shared by the conflict check and the nothing-supplied check, so both #: offer the same ways forward. _ORIGIN_HINT = ( "Navigate from a comid, e.g. comid=13294314, or from a " "feature_source/feature_id pair -- not both" ) +_ORIGIN_REMEDY = f"{_ORIGIN_HINT}." +_ORIGIN_REMEDY_NEITHER = f"{_ORIGIN_HINT}, and not neither." def _api_base() -> str: @@ -352,32 +353,6 @@ def _navigation_request( return url, {"distance": str(distance)} -def _validate_lat_long_origin( - comid: int | None, - feature_source: str | None, - feature_id: str | None, -) -> None: - """Raise if lat/long is combined with another origin type. - - Called with a lat/long already supplied, so the pair is passed as a - present marker: the conflict is between origin *types*, and naming the - type is what tells the caller which argument to drop. - """ - reject_together( - { - "lat/long": True, - "comid": comid, - "feature_source": feature_source, - "feature_id": feature_id, - }, - context="each names a different origin to navigate from", - remedy=( - "Navigate from a point (lat and long), a comid, or a " - "feature_source/feature_id pair -- one origin per call." - ), - ) - - def _get_features_request( *, data_source: str | None, @@ -398,7 +373,20 @@ def _get_features_request( ) if lat is not None: - _validate_lat_long_origin(comid, feature_source, feature_id) + # The pair is one origin, so it enters the conflict check as one entry. + reject_together( + { + "lat/long": lat, + "comid": comid, + "feature_source": feature_source, + "feature_id": feature_id, + }, + context="each names a different origin to navigate from", + remedy=( + "Navigate from a point (lat and long), a comid, or a " + "feature_source/feature_id pair -- one origin per call." + ), + ) return f"{_api_base()}/comid/position", {"coords": f"POINT({long} {lat})"} if comid is not None or data_source is not None: @@ -494,13 +482,13 @@ def _search_basin(feature_source: str | None, feature_id: str | None) -> dict[st context="for find='basin'", remedy=remedy, ) + feature_source = require_argument( + "feature_source", feature_source, context="for find='basin'", remedy=remedy + ) + # require_together above: feature_id is present iff feature_source is. return get_basin( - feature_source=require_argument( - "feature_source", feature_source, context="for find='basin'", remedy=remedy - ), - feature_id=require_argument( - "feature_id", feature_id, context="for find='basin'", remedy=remedy - ), + feature_source=feature_source, + feature_id=cast("str", feature_id), as_json=True, ) @@ -682,12 +670,7 @@ def _validate_data_source(data_source: str, *, name: str = "data source") -> Non ) _AVAILABLE_DATA_SOURCES = [ds["source"] for ds in available_data_sources] - if data_source not in _AVAILABLE_DATA_SOURCES: - err_msg = ( - f"Invalid {name} '{data_source}'." - f" Available sources are: {_AVAILABLE_DATA_SOURCES}" - ) - raise ValueError(err_msg) + require_one_of(data_source, _AVAILABLE_DATA_SOURCES, name=name) def _validate_navigation_mode(navigation_mode: str | None) -> str: @@ -714,7 +697,7 @@ def _validate_feature_source_comid( "feature_id": feature_id, }, context="they name different origins", - remedy=f"{_ORIGIN_HINT}.", + remedy=_ORIGIN_REMEDY, ) require_together( {"feature_source": feature_source, "feature_id": feature_id}, @@ -724,7 +707,7 @@ def _validate_feature_source_comid( require_exactly_one( {"comid": comid, "feature_source": feature_source}, context="as the origin to navigate from", - remedy=f"{_ORIGIN_HINT}, and not neither.", + remedy=_ORIGIN_REMEDY_NEITHER, ) diff --git a/dataretrieval/nwdc.py b/dataretrieval/nwdc.py index 2141a8141..614bf9585 100644 --- a/dataretrieval/nwdc.py +++ b/dataretrieval/nwdc.py @@ -52,7 +52,7 @@ from dataretrieval import configuration as _configuration from dataretrieval._querying import _raise_for_status, to_str from dataretrieval._response_metadata import BaseMetadata -from dataretrieval._validation import require_exactly_one +from dataretrieval._validation import render_options, require_exactly_one from dataretrieval.codes.states import to_state from dataretrieval.configuration import ( BaseConfiguration, @@ -302,8 +302,7 @@ def _resolve_locations( location string per value — the caller issues one request per location. """ selectors = {"state": state, "county": county, "huc": huc} - require_exactly_one(selectors, context="as the query's location") - [(name, value)] = ((n, v) for n, v in selectors.items() if v is not None) + name, value = require_exactly_one(selectors, context="as the query's location") locations = _LOCATION_BUILDERS[name](value) if not locations: raise ValueError( @@ -467,7 +466,7 @@ def _nwdc_error_detail(response: httpx.Response) -> str | None: return None if detail.startswith("Invalid model name"): # The service names the rejected value but not the accepted ones. - return f"{detail.rstrip('.')}. Valid models are: {', '.join(MODELS)}." + return f"{detail.rstrip('.')}. Valid models are: {render_options(MODELS)}." return detail diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 55fc5035f..fea7477a3 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -48,6 +48,16 @@ "peaks", "ratings", ] +# The major filters each query function accepts, hoisted beside the service +# lists so the checks and their remedies read from one roster. +_WATERDATA_MAJOR_FILTERS = ("site_no", "stateCd") +_WATERDATA_BBOX_CORNERS = ( + "nw_longitude_va", + "nw_latitude_va", + "se_longitude_va", + "se_latitude_va", +) +_WATERSERVICES_MAJOR_FILTERS = ("sites", "stateCd", "bBox", "huc", "countyCd") # NAD83 _CRS = "EPSG:4269" @@ -392,33 +402,26 @@ def query_waterdata( request: ``httpx.Response`` The response object from the API request to the web service. """ - major_params = ["site_no", "stateCd"] - bbox_params = [ - "nw_longitude_va", - "nw_latitude_va", - "se_longitude_va", - "se_latitude_va", - ] - require_any_of( - {name: kwargs.get(name) for name in major_params + bbox_params}, + { + name: kwargs.get(name) + for name in _WATERDATA_MAJOR_FILTERS + _WATERDATA_BBOX_CORNERS + }, context="as a major filter", remedy=( "Pass one, e.g. site_no='01491000' or stateCd='WI', or all four " "bounding-box corners together with " "coordinate_format='decimal_degrees'." ), - error=TypeError, ) require_together( - {name: kwargs.get(name) for name in bbox_params}, + {name: kwargs.get(name) for name in _WATERDATA_BBOX_CORNERS}, context="to describe a bounding box", remedy=( "Pass them along with coordinate_format='decimal_degrees', or " - f"drop the bounding box and filter with {' or '.join(major_params)} " - "instead." + "drop the bounding box and filter with " + f"{' or '.join(_WATERDATA_MAJOR_FILTERS)} instead." ), - error=TypeError, ) require_one_of( service, @@ -427,9 +430,10 @@ def query_waterdata( remedy=( "Rating tables come from waterdata.get_ratings(" "monitoring_location_id='USGS-01646500'), served from a different " - "endpoint and keyed by the AGENCY-ID form of the site number." + "endpoint and keyed by the AGENCY-ID form of the site number. It " + "returns {'USGS-01646500.exsa.rdb': DataFrame} -- a dict per file, " + "not a (frame, metadata) pair." ), - error=TypeError, ) url = WATERDATA_URL + service @@ -479,14 +483,12 @@ def query_waterservices( The response object from the API request to the web service. """ - major_filters = ["sites", "stateCd", "bBox", "huc", "countyCd"] require_any_of( - {name: kwargs.get(name) for name in major_filters}, + {name: kwargs.get(name) for name in _WATERSERVICES_MAJOR_FILTERS}, context="as a major filter", remedy=("Pass one, e.g. sites='01491000', stateCd='WI', or countyCd='55025'."), - error=TypeError, ) - require_one_of(service, WATERSERVICES_SERVICES, name="service", error=TypeError) + require_one_of(service, WATERSERVICES_SERVICES, name="service") if "format" not in kwargs: kwargs["format"] = "rdb" @@ -830,10 +832,7 @@ def get_ratings( if site is not None: payload.update({"site_no": site}) if file_type is not None: - if file_type not in ["base", "corr", "exsa"]: - raise ValueError( - f'Unrecognized file_type: {file_type}, must be "base", "corr" or "exsa"' - ) + require_one_of(file_type, ("base", "corr", "exsa"), name="file_type") payload.update({"file_type": file_type}) response = query(url, payload, ssl_check=ssl_check) return _read_rdb(response.text), NWIS_Metadata(response, site_no=site) @@ -993,7 +992,6 @@ def get_record( "New work should use the dataretrieval.waterdata getters instead; " "NWIS is deprecated." ), - error=TypeError, ) if service == "iv": @@ -1044,12 +1042,8 @@ def get_record( df, _ = get_stats(sites=sites, ssl_check=ssl_check, **kwargs) return df - else: # pragma: no cover - a recognized service with no branch above - raise TypeError( - f"The {service!r} service is recognized but get_record has no " - "handler for it. This is a bug in dataretrieval; please report it " - "at https://github.com/DOI-USGS/dataretrieval-python/issues." - ) + else: # pragma: no cover - every recognized service has a branch above + raise AssertionError(f"get_record has no handler for service {service!r}") def _site_block_boundaries(site_list: list[str]) -> list[int]: diff --git a/dataretrieval/ogc/dates.py b/dataretrieval/ogc/dates.py index 471577a8f..4db800ead 100644 --- a/dataretrieval/ogc/dates.py +++ b/dataretrieval/ogc/dates.py @@ -111,7 +111,7 @@ def _format_api_dates( date: bool = False, *, name: str = "date input", - advertise_duration: bool = True, + single_value_hint: str = "an instant or a duration ('2020-01-01', 'P7D')", ) -> str | None: """ Formats date or datetime input(s) for use with an API. @@ -135,12 +135,11 @@ def _format_api_dates( every message raised here. Defaults to a generic "date input"; pass the real parameter name (``"time"``, ``"last_modified"``) so a caller correcting the error edits an argument their getter actually accepts. - advertise_duration : bool, optional - Whether the "too many values" message offers an ISO 8601 duration as - an accepted single value. **Wording only -- this does not reject - durations.** A getter that refuses them (``get_ratings``, via - :func:`~dataretrieval.waterdata.ratings._validate_time_no_duration`) - enforces that itself and passes False here so the remedy does not + single_value_hint : str, optional + How the "too many values" message describes an acceptable single + value. Wording only -- a getter that rejects some of the default's + forms (``get_ratings`` refuses durations) enforces that itself and + passes a hint naming only what it accepts, so the remedy does not send a caller straight into its rejection. Returns @@ -182,12 +181,8 @@ def _format_api_dates( if len(items) > 2: raise ValueError( f"{name} takes at most 2 values, got {len(items)}: {items!r}. " - + ( - "Pass one value for an instant or a duration ('2020-01-01', 'P7D'), " - if advertise_duration - else "Pass one value for an instant ('2020-01-01'), " - ) - + "or two for a closed interval ('2020-01-01', '2020-12-31')." + f"Pass one value for {single_value_hint}, " + "or two for a closed interval ('2020-01-01', '2020-12-31')." ) # Pass through duration ("P7D", "PT36H") and pre-formatted interval ("a/b") diff --git a/dataretrieval/waterdata/ratings.py b/dataretrieval/waterdata/ratings.py index 1cfb23730..55b5f21e8 100644 --- a/dataretrieval/waterdata/ratings.py +++ b/dataretrieval/waterdata/ratings.py @@ -17,6 +17,7 @@ import httpx import pandas as pd +from dataretrieval._validation import render_options from dataretrieval.exceptions import DataRetrievalError, SkippedRatingWarning from dataretrieval.ogc.dates import _DURATION_RE, _format_api_dates from dataretrieval.ogc.errors import _raise_for_non_200 @@ -177,7 +178,9 @@ def get_ratings( _validate_file_types(file_types) _validate_time_no_duration(time) time_str = ( - _format_api_dates(time, name="time", advertise_duration=False) + _format_api_dates( + time, name="time", single_value_hint="an instant ('2020-01-01')" + ) if time is not None else None ) @@ -209,8 +212,8 @@ def _validate_file_types(file_types: list[str]) -> None: invalid = [ft for ft in file_types if ft not in _VALID_FILE_TYPES] if invalid: raise ValueError( - f"Invalid file_type {invalid!r}; " - f"valid options are {list(_VALID_FILE_TYPES)}." + f"Invalid file_type: {render_options(invalid)}. " + f"Valid options are: {render_options(_VALID_FILE_TYPES)}." ) diff --git a/dataretrieval/waterdata/reference.py b/dataretrieval/waterdata/reference.py index bda54bdde..242f05f60 100644 --- a/dataretrieval/waterdata/reference.py +++ b/dataretrieval/waterdata/reference.py @@ -100,7 +100,7 @@ def get_reference_table( # ``removesuffix`` rather than an ``endswith`` branch, whose non-plural arm # was unreachable and would stay correct if a singular collection appeared. if collection in ("counties", "countries"): - output_id = collection[:-3] + "y" # county / country + output_id = collection.removesuffix("ies") + "y" else: output_id = collection.removesuffix("s").replace("-", "_") diff --git a/tests/nldi_test.py b/tests/nldi_test.py index 405a90e02..e448272eb 100644 --- a/tests/nldi_test.py +++ b/tests/nldi_test.py @@ -394,7 +394,7 @@ def test_validate_data_source_rejects_invalid_after_cache_populated(httpx_mock): nldi._validate_data_source("WQP") - with pytest.raises(ValueError, match="Invalid data source 'not_a_real_source'"): + with pytest.raises(ValueError, match="Invalid data source: 'not_a_real_source'"): nldi._validate_data_source("not_a_real_source") diff --git a/tests/nwis_test.py b/tests/nwis_test.py index d43d4a781..9c424fa01 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -338,7 +338,7 @@ def test_each_service_reaches_its_own_getter(self, service, target, site_kwarg): assert out is frame def test_unrecognized_service_lists_the_ones_it_serves(self): - with pytest.raises(TypeError) as excinfo: + with pytest.raises(ValueError) as excinfo: get_record(sites="01491000", service="nope") message = str(excinfo.value) assert "Invalid service: 'nope'" in message diff --git a/tests/utils_test.py b/tests/utils_test.py index e5616ea26..04abd4523 100644 --- a/tests/utils_test.py +++ b/tests/utils_test.py @@ -449,7 +449,7 @@ def test_rejects_unrecognized_state(self): def test_rejects_unknown_target(self): from dataretrieval.codes.states import to_state - with pytest.raises(ValueError, match="to must be"): + with pytest.raises(ValueError, match="Invalid to"): to_state("WI", "zipcode") def test_resolves_an_iterable_element_wise(self): diff --git a/tests/validation_test.py b/tests/validation_test.py index f27c34d9e..96d2c5f9c 100644 --- a/tests/validation_test.py +++ b/tests/validation_test.py @@ -130,8 +130,11 @@ def test_an_explicit_none_is_not_a_filter(self): class TestRequireExactlyOne: - def test_accepts_exactly_one(self): - require_exactly_one({"comid": 1, "feature_source": None}) + def test_accepts_exactly_one_and_returns_it(self): + assert require_exactly_one({"comid": 1, "feature_source": None}) == ( + "comid", + 1, + ) def test_none_supplied_says_to_pass_one(self): with pytest.raises(ValueError) as excinfo: @@ -171,20 +174,3 @@ def test_context_explains_why_they_conflict(self): reject_together( {"lat": 1.0, "comid": 2}, context="they name different origins" ) - - -@pytest.mark.parametrize( - "check", - [ - lambda: require_one_of("x", ("a",), name="service", error=TypeError), - lambda: require_argument("service", None, error=TypeError), - lambda: require_together({"a": 1, "b": None}, error=TypeError), - lambda: require_any_of({"a": None}, error=TypeError), - lambda: require_exactly_one({"a": None, "b": None}, error=TypeError), - lambda: reject_together({"a": 1, "b": 2}, error=TypeError), - ], -) -def test_every_check_raises_the_callers_exception_class(check): - """Sharing the wording must not change what a caller already catches.""" - with pytest.raises(TypeError): - check() diff --git a/tests/waterdata_utils_test.py b/tests/waterdata_utils_test.py index d80385027..62b461545 100644 --- a/tests/waterdata_utils_test.py +++ b/tests/waterdata_utils_test.py @@ -933,7 +933,9 @@ def test_the_duration_example_is_withheld_where_durations_are_rejected(): with pytest.raises(ValueError) as allowed: _format_api_dates(["a", "b", "c"], name="time") with pytest.raises(ValueError) as refused: - _format_api_dates(["a", "b", "c"], name="time", advertise_duration=False) + _format_api_dates( + ["a", "b", "c"], name="time", single_value_hint="an instant ('2020-01-01')" + ) assert "'P7D'" in str(allowed.value) assert "'P7D'" not in str(refused.value) @@ -1315,11 +1317,11 @@ def test_with_state_routes_into_native_queryable(): def test_with_state_conflict_raises(): """Passing ``state`` together with a native ``state_code``/``state_name`` is ambiguous and raises.""" - with pytest.raises(ValueError, match="not both"): + with pytest.raises(ValueError, match="cannot be combined"): _utils_module._with_state( {"state": "WI", "state_code": "55"}, to="name", into="state_name" ) - with pytest.raises(ValueError, match="not both"): + with pytest.raises(ValueError, match="cannot be combined"): _utils_module._with_state( {"state": "WI", "state_name": "Wisconsin"}, to="name", into="state_name" ) @@ -1330,7 +1332,7 @@ def test_with_state_conflict_via_queryables_raises(): explicit getter parameter, as with ``get_time_series_metadata``'s ``state_code``) is flattened before the mutual-exclusion check, so combining it with ``state`` still raises rather than silently sending both filters.""" - with pytest.raises(ValueError, match="not both"): + with pytest.raises(ValueError, match="cannot be combined"): _utils_module._with_state( {"state": "WI", "queryables": {"state_code": "55"}}, to="name", diff --git a/tests/waterservices_test.py b/tests/waterservices_test.py index ba63ee834..291c64a15 100644 --- a/tests/waterservices_test.py +++ b/tests/waterservices_test.py @@ -30,25 +30,25 @@ def test_query_waterdata_validation(): typically a program, and what it needs from the failure is the set of values that would have been accepted. """ - with pytest.raises(TypeError) as type_error: + with pytest.raises(ValueError) as value_error: query_waterdata(service="pmcodes", format="rdb") - message = str(type_error.value) + message = str(value_error.value) assert "is required as a major filter" in message assert "site_no, stateCd" in message assert "nw_longitude_va" in message - with pytest.raises(TypeError) as type_error: + with pytest.raises(ValueError) as value_error: query_waterdata(service=None, site_no="sites") - message = str(type_error.value) + message = str(value_error.value) assert "Invalid service: None" in message # 'ratings' was advertised here but is not an NwisWeb program: the URL it # built returned an HTML error page, not data. assert "'peaks'" in message assert "get_ratings" in message - with pytest.raises(TypeError) as type_error: + with pytest.raises(ValueError) as value_error: query_waterdata(service="pmcodes", nw_longitude_va="something") - message = str(type_error.value) + message = str(value_error.value) assert "must be given together to describe a bounding box" in message # The three corners actually absent, so the caller knows what to add. assert "nw_latitude_va, se_longitude_va and se_latitude_va" in message @@ -56,15 +56,15 @@ def test_query_waterdata_validation(): def test_query_waterservices_validation(): """Tests the validation parameters of the query_waterservices method""" - with pytest.raises(TypeError) as type_error: + with pytest.raises(ValueError) as value_error: query_waterservices(service="dv", format="rdb") - message = str(type_error.value) + message = str(value_error.value) assert "is required as a major filter" in message assert "sites, stateCd, bBox, huc or countyCd" in message - with pytest.raises(TypeError) as type_error: + with pytest.raises(ValueError) as value_error: query_waterservices(service=None, sites="sites") - message = str(type_error.value) + message = str(value_error.value) assert "Invalid service: None" in message assert "'dv', 'iv', 'site', 'stat'" in message @@ -91,9 +91,9 @@ def test_query_validation(httpx_mock): def test_get_record_validation(): """An unknown service names the ones get_record does serve.""" - with pytest.raises(TypeError) as type_error: + with pytest.raises(ValueError) as value_error: get_record(sites=["01491000"], service="not_a_service") - message = str(type_error.value) + message = str(value_error.value) assert "Invalid service: 'not_a_service'" in message assert "'dv', 'iv', 'site', 'stat', 'peaks', 'ratings'" in message @@ -275,9 +275,9 @@ def test_get_ratings_validation(): site = "01594440" with pytest.raises(ValueError) as value_error: get_ratings(site=site, file_type="BAD") - assert 'Unrecognized file_type: BAD, must be "base", "corr" or "exsa"' in str( - value_error - ) + message = str(value_error.value) + assert "Invalid file_type: 'BAD'" in message + assert "'base', 'corr', 'exsa'" in message def test_get_ratings(httpx_mock):