diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index a95ffec9e..807532c89 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -145,13 +145,21 @@ 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/ - coverage report -m + run: coverage run -m pytest tests/ + - name: Coverage ratchet + # 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 + - name: Coverage report (informational) + if: runner.os == 'Windows' + shell: bash + run: coverage report -m --fail-under=0 diff --git a/AGENTS.md b/AGENTS.md index 7c821ecfd..9b0bf9b1e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,34 +1,157 @@ # 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`, 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). +- 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 shared shapes — bad value in a closed vocabulary + (`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. 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 + 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. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4416e8d76..3b5af121c 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,31 @@ 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. + +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/NEWS.md b/NEWS.md index 137ba5bf2..c9cb7177f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,7 @@ +**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 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. **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 68b1822e7..a4cdbf6b9 100644 --- a/dataretrieval/_response_metadata.py +++ b/dataretrieval/_response_metadata.py @@ -53,13 +53,16 @@ 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). @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..ff07fa25b 100644 --- a/dataretrieval/_validation.py +++ b/dataretrieval/_validation.py @@ -1,38 +1,94 @@ """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. +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`). 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" +which services exist -- so a check that cannot name a remedy 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: + +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], *, name: str, context: str = "", + remedy: str = "", ) -> None: """Raise ``ValueError`` unless *value* is one of *options*. @@ -53,6 +109,11 @@ 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 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. Raises ------ @@ -65,7 +126,230 @@ 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}. " + f"Valid options are: {render_options(options)}." + ) + raise ValueError(f"{message} {remedy}" if remedy else message) + + +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 = _qualify(context) + 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 = _qualify(context) + 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_any_of( + values: Mapping[str, object], + *, + context: str = "", + remedy: str = "", +) -> 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, so ``sites=None`` is refused rather than + reaching 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. + + Raises + ------ + ValueError + If none of *values* were supplied. + """ + supplied, _ = _supplied(values) + if supplied: + return + where = _qualify(context) + names = _render_names(values, conjunction="or") + fix = remedy or f"Pass one of {names}." + raise ValueError(f"At least one of {names} is required{where}. {fix}") + + +def require_exactly_one( + values: Mapping[str, _T | None], + *, + context: str = "", + remedy: str = "", +) -> 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 + 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 + ---------- + 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. + + 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. + """ + 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 ValueError( - f"Invalid {name}: {value!r}{qualifier}. Valid options are: {_render(options)}." + 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 = _qualify(context, prefix=" -- ") + 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..55bce3d0e 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 @@ -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", @@ -66,6 +68,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 +127,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 +162,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,9 +188,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").' + 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) @@ -185,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( @@ -211,11 +223,36 @@ 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`` 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) + 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) + raise ValueError( + 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/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..ff68486c9 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, 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 @@ -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, 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 Provider agency code. diff --git a/dataretrieval/nldi.py b/dataretrieval/nldi.py index 1649b3052..c46afd931 100644 --- a/dataretrieval/nldi.py +++ b/dataretrieval/nldi.py @@ -16,7 +16,14 @@ 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, + render_options, + require_argument, + require_exactly_one, + require_one_of, + require_together, +) from dataretrieval.configuration import ( BaseConfiguration, _Redirectable, @@ -37,12 +44,26 @@ 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") +#: Built from the tuple above, so a mode added there cannot go unmentioned. +_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: @@ -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() @@ -326,23 +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.""" - 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" - ) - - def _get_features_request( *, data_source: str | None, @@ -356,35 +366,70 @@ 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) + # 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) 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 +473,24 @@ 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( + {"feature_source": feature_source, "feature_id": feature_id}, + 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=feature_source, + feature_id=cast("str", feature_id), + as_json=True, + ) def _search_flowlines( @@ -442,11 +502,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 +596,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 +647,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 +662,23 @@ 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}" - ) - raise ValueError(err_msg) + require_one_of(data_source, _AVAILABLE_DATA_SOURCES, name=name) 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 +687,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=_ORIGIN_REMEDY, ) + 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=_ORIGIN_REMEDY_NEITHER, + ) @dataclass(frozen=True) diff --git a/dataretrieval/nwdc.py b/dataretrieval/nwdc.py index 667053f1b..614bf9585 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 render_options, require_exactly_one from dataretrieval.codes.states import to_state from dataretrieval.configuration import ( BaseConfiguration, @@ -300,21 +301,12 @@ 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} + name, value = require_exactly_one(selectors, context="as the query's location") 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 +405,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 +459,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: {render_options(MODELS)}." + return detail @dataclass(frozen=True) diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 9221513ec..fea7477a3 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,10 +42,22 @@ 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", ] +# 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" @@ -121,8 +138,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 @@ -370,7 +389,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 @@ -381,24 +402,39 @@ 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", - ] - - 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") - - 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.") - - if service not in WATERDATA_SERVICES: - raise TypeError("Service not recognized") + require_any_of( + { + 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'." + ), + ) + require_together( + {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 " + "drop the bounding box and filter with " + f"{' or '.join(_WATERDATA_MAJOR_FILTERS)} instead." + ), + ) + require_one_of( + service, + ("peaks",), + name="service", + 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. It " + "returns {'USGS-01646500.exsa.rdb': DataFrame} -- a dict per file, " + "not a (frame, metadata) pair." + ), + ) url = WATERDATA_URL + service @@ -447,15 +483,12 @@ 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"] - ): - raise TypeError( - "Query must specify a major filter: sites, stateCd, bBox, huc, or countyCd" - ) - - if service not in WATERSERVICES_SERVICES: - raise TypeError("Service not recognized") + require_any_of( + {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'."), + ) + require_one_of(service, WATERSERVICES_SERVICES, name="service") if "format" not in kwargs: kwargs["format"] = "rdb" @@ -734,8 +767,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')`." ) @@ -799,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) @@ -895,7 +925,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 +976,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 +984,15 @@ 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}") + require_one_of( + service, + WATERSERVICES_SERVICES + WATERDATA_SERVICES, + name="service", + remedy=( + "New work should use the dataretrieval.waterdata getters instead; " + "NWIS is deprecated." + ), + ) if service == "iv": df, _ = get_iv( @@ -1005,8 +1042,8 @@ 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 - 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]: @@ -1124,7 +1161,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..4db800ead 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", + 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. @@ -125,6 +130,17 @@ 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. + 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 ------- @@ -157,13 +173,17 @@ 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}. " + 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") 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..357ada2e2 100644 --- a/dataretrieval/waterdata/cql.py +++ b/dataretrieval/waterdata/cql.py @@ -46,6 +46,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]: @@ -85,7 +86,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``). @@ -138,7 +148,16 @@ def get_cql( ... ' "02070010%"]}', ... ) """ - require_one_of(collection, sorted(_OUTPUT_ID_BY_COLLECTION), name="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 @@ -159,7 +178,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/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..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 @@ -176,7 +177,13 @@ 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", single_value_hint="an instant ('2020-01-01')" + ) + 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 @@ -205,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)}." ) @@ -346,7 +353,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..242f05f60 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 @@ -95,12 +97,12 @@ 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" - elif collection.endswith("s"): - output_id = collection[:-1].replace("-", "_") + # ``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.removesuffix("ies") + "y" 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/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/pyproject.toml b/pyproject.toml index 54d8ab34a..4da15c3d3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -183,3 +183,35 @@ addopts = "-m 'not live'" markers = [ "live: hits real USGS services; deselected by default, run on a schedule", ] + +[tool.coverage.run] +source = ["dataretrieval"] +# Most of what this package gets wrong is a branch rather than a line. +branch = true +omit = [ + # Written by setuptools_scm at build time; absent from a source checkout. + "dataretrieval/_version.py", +] + +[tool.coverage.report] +show_missing = true +skip_covered = true +# 98.97% displays as "99%", which would pass a ``fail_under = 99`` on rounding. +precision = 2 +# 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 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', + # The suite installs the [test,nldi] extras, so an absent-dependency fallback + # is unreachable here by construction. + '^\s*except ImportError', + '^\s*except PackageNotFoundError', + # 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/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 b461b019e..04abd4523 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 @@ -448,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): @@ -465,6 +466,85 @@ 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("Atlantis") + message = str(excinfo.value) + assert "state_name" not in message + assert "state_code" not in message + + +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.""" + + 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": "Atlantis"}, + 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``, so 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": "Atlantis"}, to=to, into=into) + assert "instead" not in str(excinfo.value) + def test_retrying_get_maps_invalid_url(monkeypatch): """Direct active-service GETs do not leak raw httpx InvalidURL errors.""" @@ -478,3 +558,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/validation_test.py b/tests/validation_test.py index 1f9f638a7..96d2c5f9c 100644 --- a/tests/validation_test.py +++ b/tests/validation_test.py @@ -1,8 +1,21 @@ -"""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_any_of, + require_argument, + require_exactly_one, + require_one_of, + require_together, +) def test_accepts_a_valid_option(): @@ -22,9 +35,142 @@ 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: 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." + ) + 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 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 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_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: + 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_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 e16e2470e..55a0a92ed 100644 --- a/tests/waterdata_nearest_test.py +++ b/tests/waterdata_nearest_test.py @@ -415,3 +415,133 @@ 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 + + +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 f6e859a4e..8e4043575 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") @@ -1205,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") @@ -1450,3 +1499,27 @@ 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 + + +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 27e4b9b79..62b461545 100644 --- a/tests/waterdata_utils_test.py +++ b/tests/waterdata_utils_test.py @@ -903,6 +903,54 @@ 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", single_value_hint="an instant ('2020-01-01')" + ) + + 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. @@ -1269,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" ) @@ -1284,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", @@ -1345,3 +1393,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 diff --git a/tests/waterservices_test.py b/tests/waterservices_test.py index 8de81122c..291c64a15 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""" - with pytest.raises(TypeError) as type_error: + """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(ValueError) as value_error: query_waterdata(service="pmcodes", format="rdb") - assert ( - str(type_error.value) - == "Query must specify a major filter: site_no, stateCd, bBox" - ) + 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") - assert str(type_error.value) == "Service not recognized" + 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") - assert ( - str(type_error.value) == "One or more lat/long coordinates missing or invalid." - ) + 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 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") - assert ( - str(type_error.value) - == "Query must specify a major filter: sites, stateCd, bBox, huc, or countyCd" - ) + 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") - assert str(type_error.value) == "Service not recognized" + message = str(value_error.value) + assert "Invalid 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""" - with pytest.raises(TypeError) as type_error: + """An unknown service names the ones get_record does serve.""" + with pytest.raises(ValueError) as value_error: get_record(sites=["01491000"], service="not_a_service") - assert str(type_error.value) == "Unrecognized service: not_a_service" + message = str(value_error.value) + assert "Invalid 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): @@ -251,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):