diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index 807532c8..99c90eed 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -20,8 +20,7 @@ jobs: python-version: "3.14" cache: "pip" - name: Install ruff - # Keep this version aligned with the ruff-pre-commit revision. - run: pip install ruff==0.16.1 + run: pip install -e .[lint] - name: Lint with ruff run: | ruff check . --output-format=github diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6ae92522..82eaa1be 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -21,7 +21,7 @@ repos: - id: debug-statements - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.16.1 + rev: v0.16.5 hooks: - id: ruff-check args: [--fix] diff --git a/AGENTS.md b/AGENTS.md index 8a19b7b4..f85ffdf4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,8 +63,8 @@ can predict where a thing lives. - Python >= 3.10; the CI test matrix is 3.10, 3.13, 3.14. ## Commands -- Lint: `ruff check .` and `ruff format --check .` (pinned to the version in - `.pre-commit-config.yaml` and the CI lint job — keep them aligned). +- Lint: `ruff check .` and `ruff format --check .` (`pip install -e .[lint]` + pins the version; keep the `.pre-commit-config.yaml` rev aligned with it). - 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 diff --git a/NEWS.md b/NEWS.md index 29ec9a62..c6e51bfe 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,5 @@ +**08/30/2026:** **Bug fix:** `waterdata.get_ratings(..., file_path=...)` wrote each rating with no explicit encoding, so on a non-UTF-8 locale an unrepresentable character raised `UnicodeEncodeError` and the rating was silently dropped from the returned dict; ratings are now written as UTF-8, byte for byte with the response. **Bug fix:** that same write blocked the event loop, so on a multi-location request every other in-flight download stalled for its duration; it now runs in a worker thread. + **08/27/2026:** **Bug fix:** `nwis.get_discharge_peaks` and `nwis.get_record(service='peaks')` silently discarded every peak whose date is only partly known. NWIS zero-fills the unknown part of a historical peak's date -- `YYYY-MM-00` when the day is not known, `YYYY-00-00` when the month is not either (the `Bd` and `Bm` `peak_cd` qualifiers) -- and neither parses as a date, so `preformat_peaks_response` coerced both to `NaT` and then dropped the row along with its discharge value. These are real peaks, and disproportionately a site's largest: site 14105700 lost 20 of its 167 peaks, among them an 1859 flood of 847,000 ft3/s, and 06934500 lost its 1844 peak of 700,000 ft3/s. Such a peak is now kept, with `datetime` left as `NaT`. The date is not completed into one NWIS does not have: a `datetime64` column cannot hold a partial date, so any value there would assert a day the record does not claim. **Behavior change:** peaks queries return more rows than before, and `datetime` may now be `NaT` -- a caller selecting on the datetime index will not see those peaks and should filter on `peak_dt` instead. **Behavior change:** `peak_dt` is no longer removed from the returned frame. It is the only column carrying a censored peak's year, since the peaks response has no `water_yr`, and the only dependable way to tell an unknown day from a known one -- `peak_cd` does not always carry the qualifier (22 of 24 censored dates across six sites tested). For peaks with a resolved timestamp alongside explicit `year`/`month`/`day` and a `qualifier` field, use `waterdata.get_peaks()`. **08/26/2026:** **Bug fix:** `nwis.format_response(df, service='peaks')` and `nwis.preformat_peaks_response` raised `KeyError('peak_dt')` on an empty peaks response instead of returning an empty frame. Both are public, and every other service already treated an empty result as a legitimate empty frame rather than an error (issue #171); the peaks arm slipped through because it reformats the datetime column before the empty-frame check. Callers can now check `df.empty` rather than catching an exception. A *non-empty* frame with no `peak_dt` column is malformed rather than empty, and still raises. diff --git a/dataretrieval/codes/__init__.py b/dataretrieval/codes/__init__.py index a1b0e400..e0677d5f 100644 --- a/dataretrieval/codes/__init__.py +++ b/dataretrieval/codes/__init__.py @@ -1,2 +1,9 @@ +"""Facade over the ``states`` and ``timezones`` lookup tables. + +Re-exports the state code maps and their normalizers (``to_state``, +``apply_state``) alongside the ``tz`` UTC-offset map, so one import +reaches every code lookup in the package. +""" + from .states import * from .timezones import * diff --git a/dataretrieval/configuration.py b/dataretrieval/configuration.py index 4c6992bc..ca797f2c 100644 --- a/dataretrieval/configuration.py +++ b/dataretrieval/configuration.py @@ -819,7 +819,7 @@ def _accepts(adapter: str, name: str) -> bool: return name in _ALL_SETTINGS if accepted is None else name in accepted -def _display_api_key(adapter: str | None = None) -> str: +def _display_api_key(_adapter: str | None = None) -> str: """Render the key's presence, never its value.""" return "" if api_key() else "" @@ -829,7 +829,7 @@ def _display_concurrency(adapter: str | None = None) -> str: return CONCURRENCY_UNBOUNDED if value is None else str(value) -def _display_progress(adapter: str | None = None) -> str: +def _display_progress(_adapter: str | None = None) -> str: setting = progress() return "auto" if setting is None else ("on" if setting else "off") diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 4ef7b01a..4bdca115 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -146,15 +146,15 @@ def _parse_json_or_raise(response: httpx.Response) -> pd.DataFrame: raise -def _localize_datetime_index(df: pd.DataFrame) -> pd.DataFrame: +def _localized_datetime_index(index: pd.Index) -> pd.Index: """Localize a naive datetime index (or multi-index level) to UTC.""" - if hasattr(df.index, "levels"): + if hasattr(index, "levels"): # Multi-index: localize the datetime level (level 1) - if hasattr(df.index.levels[1], "tzinfo") and df.index.levels[1].tzinfo is None: - df = df.tz_localize("UTC", level=1) - elif hasattr(df.index, "tzinfo") and df.index.tzinfo is None: - df = df.tz_localize("UTC") - return df + if hasattr(index.levels[1], "tzinfo") and index.levels[1].tzinfo is None: + return index.set_levels(index.levels[1].tz_localize("UTC"), level=1) + elif hasattr(index, "tzinfo") and index.tzinfo is None: + return index.tz_localize("UTC") + return index def format_response( @@ -196,11 +196,19 @@ def format_response( return df if len(df["site_no"].unique()) > 1 and mi: - df.set_index(["site_no", "datetime"], inplace=True) + keys = ["site_no", "datetime"] else: - df.set_index(["datetime"], inplace=True) + keys = ["datetime"] + + # Index our own frame, never the caller's. The shallow copy shares the + # columns; ``set_index`` without ``inplace`` duplicates them, because + # pandas deep-copies the frame whenever copy-on-write is off. + df = df.copy(deep=False) + df.set_index(keys, inplace=True) # noqa: PD002 # our copy, not the caller's - df = _localize_datetime_index(df) + # Retag the index alone; ``DataFrame.tz_localize`` relabels the axis by + # duplicating every column whenever copy-on-write is off. + df.index = _localized_datetime_index(df.index) return df.sort_index() @@ -236,6 +244,8 @@ def preformat_peaks_response(df: pd.DataFrame) -> pd.DataFrame: # still raise. return df + # Derive the column on our own frame, never the caller's. + df = df.copy(deep=False) df["datetime"] = pd.to_datetime(df["peak_dt"], errors="coerce") return df @@ -895,6 +905,41 @@ def what_sites( return df, NWIS_Metadata(response, **kwargs) +# The value that means "not passed" -- which must stay in step with +# ``get_record``'s declared default -- and the replacement to name. +_DEFUNCT_RECORD_OPTIONS: dict[str, tuple[object, str]] = { + "wide_format": (True, "`waterdata.get_samples()`"), + "datetime_index": ( + True, + "`waterdata.get_continuous()` or `waterdata.get_daily()`", + ), + "state": (None, "`nwdc.get_wateruse(state=...)`"), +} + + +def _warn_defunct_record_options(**given: object) -> None: + """Advise on each ``get_record`` option asked to do something it cannot. + + Naming an option at its declared default is silent: the caller is asking + for what the dead default already gave them. + """ + for name, value in given.items(): + unset, replacement = _DEFUNCT_RECORD_OPTIONS[name] + if value != unset: + warn_deprecated( + f"`nwis.get_record`'s `{name}` argument", + replacement=replacement, + removal=_NWIS_REMOVAL_DATE, + detail=( + "It is ignored, and has been since the service that read " + "it was retired." + ), + # _warn_defunct_record_options -> get_record -> @_deprecated + # wrapper -> the caller's own line. + stacklevel=4, + ) + + @_deprecated def get_record( sites: list[str] | str | None = None, @@ -927,12 +972,16 @@ def get_record( If False, return a dataframe with a single-level index (datetime). Default is True. wide_format : bool, optional - If True, return data in wide format, with multiple samples per row and - one row per time. Default is True. + (defunct) Shaped the output of the retired 'qwdata' service. Ignored; + passing `False` warns. Use `waterdata.get_samples`, which returns one + row per result. datetime_index : bool, optional - If True, create a datetime index. Default is True. + (defunct) Shaped the output of the retired 'qwdata' and 'gwlevels' + services. Ignored; passing `False` warns. Use `waterdata.get_continuous` + or `waterdata.get_daily`, which return `time` as a column. state: string, optional, default is None - State full name, abbreviation, or id. + (defunct) Selected sites for the retired 'water_use' service. Ignored; + passing a state warns. Use `nwdc.get_wateruse`, which takes `state`. service: string, default is 'iv' - 'iv' : instantaneous data - 'dv' : daily mean data @@ -1011,6 +1060,10 @@ def get_record( ), ) + _warn_defunct_record_options( + wide_format=wide_format, datetime_index=datetime_index, state=state + ) + if service == "iv": df, _ = get_iv( sites=sites, @@ -1097,15 +1150,13 @@ def _parse_parameter_record( record_df["qualifiers"] = ( record_df["qualifiers"].astype(str).str.strip("[]").str.replace("'", "") ) - record_df.rename( + return record_df.rename( columns={ "value": col_name, "dateTime": "datetime", "qualifiers": col_name + "_cd", - }, - inplace=True, + } ) - return record_df def _parse_site_block(site_block: list[dict[str, Any]]) -> pd.DataFrame: diff --git a/dataretrieval/ogc/dates.py b/dataretrieval/ogc/dates.py index 4db800ea..6739f13a 100644 --- a/dataretrieval/ogc/dates.py +++ b/dataretrieval/ogc/dates.py @@ -53,7 +53,9 @@ def _parse_datetime(value: str) -> datetime | None: candidate = value[:-1] + "+00:00" if value.endswith("Z") else value for fmt in _DATETIME_FORMATS: try: - return datetime.strptime(candidate, fmt) + # DTZ007: naive is the documented outcome for a naive input -- + # ``_DATETIME_FORMATS`` carries both the ``%z`` and the bare forms. + return datetime.strptime(candidate, fmt) # noqa: DTZ007 except ValueError: continue return None diff --git a/dataretrieval/transport/pagination.py b/dataretrieval/transport/pagination.py index 721c81b4..1d335ecb 100644 --- a/dataretrieval/transport/pagination.py +++ b/dataretrieval/transport/pagination.py @@ -116,7 +116,7 @@ def report_page(page: httpx.Response, frame: pd.DataFrame) -> None: try: frame, cursor = parse_response(response) - except Exception as exc: # noqa: BLE001 + except Exception as exc: logger.warning("Initial response parse failed.") raise DataRetrievalError( paginated_failure_message(0, exc, response.url) @@ -141,7 +141,7 @@ def report_page(page: httpx.Response, frame: pd.DataFrame) -> None: nrows += len(frame) total_elapsed += _safe_elapsed(response) report_page(response, frame) - except Exception as exc: # noqa: BLE001 + except Exception as exc: logger.warning( "Request failed at cursor %r. Data download interrupted.", cursor ) diff --git a/dataretrieval/transport/retry.py b/dataretrieval/transport/retry.py index d38d6145..6c2e10fb 100644 --- a/dataretrieval/transport/retry.py +++ b/dataretrieval/transport/retry.py @@ -292,7 +292,7 @@ async def attempt_once() -> _T: while True: try: return await attempt_once() - except Exception as exc: # noqa: BLE001 - re-raised unless retryable + except Exception as exc: attempt += 1 wait = _retry_delay(exc, attempt, policy) if wait is None: @@ -314,7 +314,7 @@ def retry_sync(fn: Callable[[], _T], policy: RetryPolicy | None = None) -> _T: while True: try: return fn() - except Exception as exc: # noqa: BLE001 - re-raised unless retryable + except Exception as exc: attempt += 1 wait = _retry_delay(exc, attempt, policy) if wait is None: diff --git a/dataretrieval/utils.py b/dataretrieval/utils.py index 924f8881..f9dc52b7 100644 --- a/dataretrieval/utils.py +++ b/dataretrieval/utils.py @@ -19,9 +19,9 @@ import dataretrieval._querying as _querying import dataretrieval.transport.http as _transport_http -from dataretrieval._ambient import Ambient # noqa: F401 - compatibility re-export +from dataretrieval._ambient import Ambient # noqa: F401 # compatibility re-export from dataretrieval._response_metadata import ( - BaseMetadata, # noqa: F401 — compatibility re-export; defined there now + BaseMetadata, # noqa: F401 # compatibility re-export; defined there now ) from dataretrieval.codes import tz diff --git a/dataretrieval/waterdata/ratings.py b/dataretrieval/waterdata/ratings.py index 4c1b6ea6..9c5b8033 100644 --- a/dataretrieval/waterdata/ratings.py +++ b/dataretrieval/waterdata/ratings.py @@ -14,6 +14,7 @@ from collections.abc import Iterable from typing import Any, Literal, get_args +import anyio import httpx import pandas as pd @@ -340,6 +341,16 @@ def _inert_response( return httpx.Response(status, headers=headers, request=httpx.Request("GET", url)) +def _write_rating(path: str, body: str) -> None: + """Persist one rating to disk, off the event loop. + + ``_fetch_rating`` runs concurrently under a fan-out drive, so a blocking + write here would stall every other in-flight download for its duration. + """ + with open(path, "w", encoding="utf-8", newline="") as f: + f.write(body) + + async def _fetch_rating( feature: dict[str, Any], file_path: str | None ) -> tuple[pd.DataFrame, httpx.Response]: @@ -366,8 +377,9 @@ async def _fetch_rating( _raise_for_non_200(response) if file_path is not None: - with open(os.path.join(file_path, fid), "w") as f: - f.write(response.text) + await anyio.to_thread.run_sync( + _write_rating, os.path.join(file_path, fid), response.text + ) df = read_rdb(response.text) df.attrs["comment"] = extract_rdb_comment(response.text) diff --git a/dataretrieval/waterdata/types.py b/dataretrieval/waterdata/types.py index c234513d..2c774da4 100644 --- a/dataretrieval/waterdata/types.py +++ b/dataretrieval/waterdata/types.py @@ -1,3 +1,18 @@ +"""The Water Data type vocabularies the getters validate their arguments against. + +Each ``Literal`` alias here is one closed vocabulary: ``CODE_SERVICES`` for +the Samples code services, ``METADATA_COLLECTIONS`` for the reference-table +collections, ``SERVICES`` and ``PROFILES`` for the Samples resources and the +output profiles they offer, and ``WATERDATA_COLLECTIONS`` (permanent alias +``WATERDATA_SERVICES``) for the collections ``get_cql`` queries. +``PROFILE_LOOKUP`` pairs ``SERVICES`` with ``PROFILES``, mapping each Samples +resource to the profiles valid for it. + +They live apart from the getters that check them so a caller may annotate an +argument with the same alias the getter validates it against, and so one +vocabulary cannot be spelled two ways in two modules. +""" + from typing import Literal, get_args from dataretrieval._validation import require_one_of diff --git a/dataretrieval/wateruse.py b/dataretrieval/wateruse.py index 35346a61..341ab9bf 100644 --- a/dataretrieval/wateruse.py +++ b/dataretrieval/wateruse.py @@ -31,7 +31,7 @@ from dataretrieval import nwdc as _nwdc from dataretrieval._deprecation import REMOVALS, warn_deprecated -from dataretrieval.nwdc import * # noqa: F403 (re-export the public surface) +from dataretrieval.nwdc import * # noqa: F403 # re-export the public surface #: When the alias may be deleted. Read from the shared horizon table rather #: than spelled here, so it is audited and bumped with every other published diff --git a/docs/source/conf.py b/docs/source/conf.py index b129d35b..46b19f7f 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -1,3 +1,5 @@ +"""Sphinx build configuration.""" + # If extensions (or modules to document with autodoc) are in another directory, # add these directories to sys.path here. If the directory is relative to the # documentation root, use os.path.abspath to make it absolute, like shown here. diff --git a/pyproject.toml b/pyproject.toml index 4da15c3d..73d461cf 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -56,6 +56,9 @@ failed = true type-check = [ "mypy", ] +lint = [ + "ruff==0.16.5", +] # Structural gates (complexity ratchets, dependency contracts) and the wily # trend history. Kept out of ``test`` so the test job stays lean; see # CONTRIBUTING.md for what each does. @@ -77,7 +80,7 @@ test = [ "pytest-rerunfailures", "coverage", "pytest-httpx", - "ruff==0.16.1", + "dataretrieval[lint]", # ruff, pinned once in the lint extra "dataretrieval[type-check]", # mypy, pinned once in the type-check extra ] doc = [ @@ -112,16 +115,17 @@ write_to = "dataretrieval/_version.py" [tool.ruff] target-version = "py310" +line-length = 88 extend-exclude = ["demos"] [tool.ruff.lint] preview = true +explicit-preview-rules = true # Select rules: Pyflakes(F), pycodestyle(E,W), isort(I), pyupgrade(UP), # flake8-bugbear(B), flake8-quotes(Q), flake8-simplify(SIM), flake8-tidy-imports(TID) select = [ "F", "E", "W", "I", "UP", "B", "Q", "SIM", "TID", "C90", # mccabe - "E501", # line-length # A first-party import used only in annotations is not a runtime # dependency, and the project already says so twice: ``.importlinter`` sets # ``exclude_type_checking_imports = True`` ("Contracts describe what runs") @@ -132,17 +136,49 @@ select = [ # here, so they stay unselected. "TC001", "TC006", # quoted first argument to ``typing.cast`` + # A blind ``except Exception`` skips ADR 0004's transient-versus-fatal + # judgement. ``progress.py``'s best-effort handlers carry a ``noqa``. + "BLE001", + # A named parameter the body never reads is a promise nothing keeps. + # ARG002-004 have nothing to catch (the public interface is functions); + # ARG005 fires on lambdas whose parameters the call site dictates. + "ARG001", + # ``open()`` in text mode without ``encoding=`` writes in the process + # locale -- cp1252 on the Windows leg. Preview-only; recheck on a bump. + "PLW1514", + # ``automodule ... :members:`` renders a docstring-less public symbol as a + # bare signature. The rest of pydocstyle stays off: its section rules read + # an indented ``Examples:`` as a section header and "fix" it into garbage. + "D100", "D101", "D102", "D103", "D104", + # A ``# noqa`` for a rule that no longer fires. Author note: ruff's fix + # reads the rest of the line as the directive's description, so spell a + # co-located suppression ``# noqa: X # pylint: disable=...``. + "RUF100", + # A blocking call inside ``async def`` stalls the whole event loop; the + # suite awaits mocked transports, so a stalled loop still passes. + "ASYNC", + # ``inplace=`` is on pandas' way out; a naive ``datetime`` in a time-series + # library is a bug waiting for a reader in another zone. + "PD", "DTZ", + # Zero findings today: comprehensions, lazy ``%s`` logging, no ``print`` in + # a library, ``from __future__ import annotations``, retired numpy aliases. + "C4", "LOG", "G", "T20", "FA", "NPY", ] ignore = [ "SIM105", # Use `contextlib.suppress(...)` instead of `try-except-pass` "SIM117", # Use a single `with` statement with multiple contexts ] +[tool.ruff.lint.flake8-unused-arguments] +# The defunct ``nwis`` stubs absorb the old call through an unread +# ``**kwargs``; that is the point, so keep ARG001 on named parameters. +ignore-variadic-names = true + [tool.ruff.lint.mccabe] max-complexity = 20 [tool.ruff.lint.per-file-ignores] -"tests/*" = ["SIM108"] +"tests/*" = ["SIM108", "ARG001", "D100", "D101", "D102", "D103", "D104"] "**/__init__.py" = ["F403"] [tool.ruff.format] @@ -164,13 +200,6 @@ files = ["dataretrieval"] strict = true ignore_missing_imports = true -# anyio ships ``py.typed``, so mypy follows into its source — which uses -# ``match`` statements (3.10+). Under our ``python_version = "3.9"`` target that -# is a hard parse error, so don't follow anyio's source; treat it as ``Any``. -[[tool.mypy.overrides]] -module = ["anyio", "anyio.*"] -follow_imports = "skip" - [tool.pytest.ini_options] # The suite is offline by default. Every HTTP call is mocked (see # ``tests/conftest.py``), so a push neither depends on USGS uptime nor spends diff --git a/setup.py b/setup.py index 60684932..ff791617 100644 --- a/setup.py +++ b/setup.py @@ -1,3 +1,5 @@ +"""Shim so ``pip install -e .`` works; the build is configured in pyproject.toml.""" + from setuptools import setup setup() diff --git a/tests/nldi_test.py b/tests/nldi_test.py index e448272e..7eaa8440 100644 --- a/tests/nldi_test.py +++ b/tests/nldi_test.py @@ -65,7 +65,7 @@ def test_query_nldi_opts_into_retry(monkeypatch): def mock_request(httpx_mock, request_url, file_path): - with open(file_path) as text: + with open(file_path, encoding="utf-8") as text: httpx_mock.add_response( method="GET", url=request_url, @@ -518,7 +518,7 @@ def test_a_configured_base_url_redirects_every_nldi_request(httpx_mock): httpx_mock.add_response( method="GET", url=f"{mirror}/", json=[{"source": "WQP"}, {"source": "comid"}] ) - with open("tests/data/nldi_get_basin.json") as body: + with open("tests/data/nldi_get_basin.json", encoding="utf-8") as body: httpx_mock.add_response( method="GET", url=( diff --git a/tests/nwis_test.py b/tests/nwis_test.py index 49ea537a..0482aa14 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -1,3 +1,4 @@ +import inspect import json import re import warnings @@ -8,9 +9,12 @@ import pandas as pd import pytest +import dataretrieval from dataretrieval import nwis from dataretrieval.exceptions import DataCurrencyWarning from dataretrieval.nwis import ( + _DEFUNCT_RECORD_OPTIONS, + _REPLACEMENTS, NWIS_Metadata, _read_rdb, format_response, @@ -35,6 +39,25 @@ _SITE_RE = re.compile(r"^https://waterservices\.usgs\.gov/nwis/site(\?.*)?$") +# Every concrete ``module.function(args)`` the deprecation tables name, so the +# tripwire below is derived from what ships rather than from a hand-kept list. +# The prose entries (``waterdata.get_*()``) do not name a function and so do +# not match. +_NAMED_REPLACEMENTS = sorted( + set( + re.findall( + r"`(\w+)\.(\w+)\(([^`]*)\)`", + " ".join( + [ + *_REPLACEMENTS.values(), + *(r for _, r in _DEFUNCT_RECORD_OPTIONS.values()), + ] + ), + ) + ) +) + + def _load_mock_json(file_name): """Helper to load mock JSON from tests/data.""" path = Path(__file__).parent / "data" / file_name @@ -201,32 +224,102 @@ def test_nested_calls_emit_one_warning(self, httpx_mock): assert len(deprecations) == 1 assert "get_record" in str(deprecations[0].message) + @pytest.mark.parametrize("module_name, func_name, arguments", _NAMED_REPLACEMENTS) + def test_named_replacement_resolves(self, module_name, func_name, arguments): + """Tripwire: following a deprecation message literally must produce a + real call, so a user migrating doesn't hit AttributeError or TypeError. + + Fails loudly if a message lands before its referenced replacement does + (e.g. before `get_peaks` from #267). + """ + func = getattr(getattr(dataretrieval, module_name), func_name, None) + assert callable(func), ( + f"`{module_name}.{func_name}` is missing — fix the replacement " + "tables in nwis.py or add the replacement before merging." + ) + for keyword in re.findall(r"(\w+)=", arguments): + assert keyword in inspect.signature(func).parameters + + +class TestDefunctRecordOptions: + """``get_record``'s three inert options advise; they do not raise. + + They are documented parameters of a Production/Stable getter, so they + follow the published deprecation policy and go when `nwis` does, rather + than on a release of their own. + """ + @pytest.mark.parametrize( - "name", + "option, value, replacement", [ - "get_daily", - "get_continuous", - "get_monitoring_locations", - "get_stats_por", - "get_stats_date_range", - "get_peaks", - "get_ratings", + ("wide_format", False, "waterdata.get_samples"), + ("datetime_index", False, "waterdata.get_continuous"), + ("state", "OH", "nwdc.get_wateruse"), ], ) - def test_named_replacement_exists_in_waterdata(self, name): - """Tripwire: every concrete `waterdata.*` named in a deprecation message - must actually exist, so a user following the migration guidance doesn't - hit AttributeError. - - Fails loudly if this PR ever lands before its referenced replacement - does (e.g. before `get_peaks` from #267). + def test_passing_one_advises_and_still_returns_data( + self, httpx_mock, option, value, replacement + ): + _mock_site(httpx_mock) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always", DeprecationWarning) + df = get_record(sites="01491000", service="site", **{option: value}) + assert not df.empty + assert [ + w + for w in caught + if option in str(w.message) and replacement in str(w.message) + ] + + @pytest.mark.parametrize("option", sorted(_DEFUNCT_RECORD_OPTIONS)) + def test_naming_an_option_at_its_default_is_silent(self, httpx_mock, option): + """Passing the declared default asks for nothing the dead option + cannot give, so it earns no warning -- and the table's "unset" value + has to be that declared default for the distinction to hold. """ - import dataretrieval.waterdata as wd + default = inspect.signature(get_record).parameters[option].default + assert _DEFUNCT_RECORD_OPTIONS[option][0] == default + _mock_site(httpx_mock) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always", DeprecationWarning) + get_record(sites="01491000", service="site", **{option: default}) + assert not [w for w in caught if f"`{option}` argument" in str(w.message)] - assert callable(getattr(wd, name, None)), ( - f"`waterdata.{name}` is missing — fix `_REPLACEMENTS` in nwis.py " - "or add the replacement before merging." - ) + def test_defaults_advise_nothing(self, httpx_mock): + """A caller who never named an option must not be told about one.""" + _mock_site(httpx_mock) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always", DeprecationWarning) + get_record(sites="01491000", service="site") + assert not [w for w in caught if "argument is deprecated" in str(w.message)] + + def test_each_advisory_is_emitted_once_per_call(self, httpx_mock): + """A call naming all three options emits four ``DeprecationWarning``s: + one for ``get_record`` itself, and one per named option, each with a + distinct subject and a distinct replacement. + """ + _mock_site(httpx_mock) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always", DeprecationWarning) + get_record( + sites="01491000", + service="site", + wide_format=False, + datetime_index=False, + state="OH", + ) + deprecations = [w for w in caught if issubclass(w.category, DeprecationWarning)] + messages = [str(w.message) for w in deprecations] + assert len(messages) == 4 + for subject in ( + "`wide_format` argument", + "`datetime_index` argument", + "`state` argument", + ): + assert sum(subject in m for m in messages) == 1 + # Pins the advisories' hand-counted ``stacklevel``: every one must + # blame the caller's own line, not a frame inside the package. + assert {Path(w.filename).name for w in deprecations} == {Path(__file__).name} class TestDefunct: @@ -375,6 +468,61 @@ def test_no_peaks_flows_through_format_response(self): assert df.empty +class TestFormatResponseArgument: + """``format_response`` indexes a copy, so its argument survives the call. + + The function is public, and both internal callers still hold the frame + they passed while it runs. Indexing that frame in place moved their + columns into an index they never asked for. + """ + + @staticmethod + def _frame(sites): + return pd.DataFrame( + { + "site_no": sites, + "datetime": pd.date_range("2020-01-01", periods=len(sites), freq="D"), + "00060": np.arange(float(len(sites))), + } + ) + + @pytest.mark.parametrize( + "sites,expected_index", + [ + pytest.param(["01", "01", "01"], pd.DatetimeIndex, id="single-site"), + pytest.param(["01", "02", "03"], pd.MultiIndex, id="multi-site"), + ], + ) + def test_it_keeps_its_columns_and_index(self, sites, expected_index): + df = self._frame(sites) + before = df.copy(deep=True) + + out = format_response(df) + + assert isinstance(out.index, expected_index), "the result must be indexed" + pd.testing.assert_frame_equal(df, before) + + def test_the_peaks_path_keeps_them_too(self): + """The peaks arm derives ``datetime`` from ``peak_dt``, and the derived + column belongs to the result rather than to the frame it was handed. + ``preformat_peaks_response`` is public, so a caller reaches that + derivation without going through ``format_response`` at all. + """ + df = pd.DataFrame( + { + "site_no": ["01", "01"], + "peak_dt": ["2020-01-01", "2020-01-02"], + "peak_va": [1.0, 2.0], + } + ) + before = df.copy(deep=True) + + out = format_response(df, service="peaks") + + assert isinstance(out.index, pd.DatetimeIndex), "the result must be indexed" + pd.testing.assert_frame_equal(df, before) + + class TestGetRecordDispatch: """``get_record`` is a router; each service must reach its own getter. @@ -465,12 +613,9 @@ def test_utc_localization_of_a_single_datetime_index(): """NWIS returns naive local timestamps; a frame whose index is a plain DatetimeIndex must still come back tz-aware, or two services' frames cannot be concatenated.""" - df = pd.DataFrame( - {"x": [1, 2]}, - index=pd.to_datetime(["2018-01-24 10:30", "2018-01-24 11:30"]), - ) - out = nwis._localize_datetime_index(df) - assert str(out.index.tz) == "UTC" + index = pd.to_datetime(["2018-01-24 10:30", "2018-01-24 11:30"]) + out = nwis._localized_datetime_index(index) + assert str(out.tz) == "UTC" def test_metadata_site_info_is_none_when_no_site_filter_was_used(): @@ -492,8 +637,8 @@ def test_utc_localization_of_a_multi_index_datetime_level(): ], names=["site_no", "datetime"], ) - out = nwis._localize_datetime_index(pd.DataFrame({"x": [1, 2]}, index=idx)) - assert str(out.index.levels[1].tz) == "UTC" + out = nwis._localized_datetime_index(idx) + assert str(out.levels[1].tz) == "UTC" class TestGetInfoSeriesCatalog: diff --git a/tests/waterdata_chunking_test.py b/tests/waterdata_chunking_test.py index 569c1380..73d7d8fb 100644 --- a/tests/waterdata_chunking_test.py +++ b/tests/waterdata_chunking_test.py @@ -2173,7 +2173,7 @@ async def fetch(_args): try: raise httpx.ConnectError("name not known") except httpx.ConnectError: - raise ValueError("deterministic bug") # noqa: B904 - regression shape + raise ValueError("deterministic bug") # noqa: B904 # regression shape with pytest.raises(ValueError, match="deterministic bug"): fetch({"sites": ["S1"]}) diff --git a/tests/waterdata_ratings_test.py b/tests/waterdata_ratings_test.py index 598c5316..46e81763 100644 --- a/tests/waterdata_ratings_test.py +++ b/tests/waterdata_ratings_test.py @@ -136,6 +136,32 @@ def test_get_ratings_attaches_rdb_comment_and_url(httpx_mock, tmp_path): assert df.attrs["url"] == _GOOD_ASSET +def test_get_ratings_writes_the_bytes_the_service_sent(httpx_mock, tmp_path): + """The saved file must be the response body, UTF-8, byte for byte. + + Text mode with no ``encoding`` writes in the process locale, and the + resulting ``UnicodeEncodeError`` is a ``ValueError``, which the + per-feature handler downgrades to a skip -- so the rating would go + missing rather than fail. Text mode also rewrites line endings. + """ + # U+2103 is absent from cp1252, so on the Windows leg of the matrix an + # unencodable character and a rewritten line ending both land here. + body = _SAMPLE_RDB.replace("\n", "\r\n").replace("DEP", "DEP \N{DEGREE CELSIUS}") + httpx_mock.add_response( + method="GET", url=STAC_SEARCH_RE, json=_stub_search_response() + ) + httpx_mock.add_response(method="GET", url=_GOOD_ASSET, text=body) + + out = get_ratings( + monitoring_location_id="USGS-01104475", + file_type="exsa", + file_path=str(tmp_path), + ) + + assert "USGS-01104475.exsa.rdb" in out + assert (tmp_path / "USGS-01104475.exsa.rdb").read_bytes() == body.encode("utf-8") + + def test_get_ratings_download_and_parse_false_returns_features(httpx_mock): httpx_mock.add_response( method="GET", diff --git a/tests/waterdata_test.py b/tests/waterdata_test.py index d17ef585..53e19da2 100644 --- a/tests/waterdata_test.py +++ b/tests/waterdata_test.py @@ -78,7 +78,7 @@ def mock_request(httpx_mock, request_url, file_path): """Mock request code""" - with open(file_path) as text: + with open(file_path, encoding="utf-8") as text: httpx_mock.add_response( method="GET", url=request_url, @@ -291,7 +291,7 @@ def value_for(snake): return [-90.0, 30.0, -89.0, 31.0] return "x" - with open("tests/data/samples_results.txt") as fh: + with open("tests/data/samples_results.txt", encoding="utf-8") as fh: body = fh.read() # one mocked response per call; match any URL so both requests are captured. httpx_mock.add_response(text=body, headers={"mock_header": "v"}) @@ -1340,7 +1340,7 @@ def test_get_stats_por(httpx_mock): } assert len(df) == 11 assert df.loc[df["computation"] == "minimum", "percentile"].tolist() == [0.0] - assert df.loc[df["computation"] == "arithmetic_mean", "percentile"].isnull().all() + assert df.loc[df["computation"] == "arithmetic_mean", "percentile"].isna().all() def test_get_stats_por_expanded_false(httpx_mock): diff --git a/tests/waterservices_test.py b/tests/waterservices_test.py index 0fda62d8..336ebab2 100644 --- a/tests/waterservices_test.py +++ b/tests/waterservices_test.py @@ -367,7 +367,7 @@ def test_get_stats_site_value_types(httpx_mock, site_input_type_list): def mock_request(httpx_mock, request_url, file_path): - with open(file_path) as text: + with open(file_path, encoding="utf-8") as text: httpx_mock.add_response( method="GET", url=request_url, @@ -384,7 +384,7 @@ def assert_metadata(httpx_mock, request_url, md, site, parameter_cd, format): site_request_url = ( f"https://waterservices.usgs.gov/nwis/site?sites={site}&format=rdb" ) - with open("tests/data/waterservices_site.txt") as text: + with open("tests/data/waterservices_site.txt", encoding="utf-8") as text: httpx_mock.add_response( method="GET", url=site_request_url, text=text.read() ) diff --git a/tests/wqp_test.py b/tests/wqp_test.py index 09b9e3ac..c2022383 100644 --- a/tests/wqp_test.py +++ b/tests/wqp_test.py @@ -25,7 +25,7 @@ def mock_request(httpx_mock, request_url, file_path): - with open(file_path) as text: + with open(file_path, encoding="utf-8") as text: httpx_mock.add_response( method="GET", url=request_url, @@ -137,7 +137,7 @@ def test_what_activities_accepts_documented_legacy_profiles(httpx_mock, profile) def test_wqx3_get_results_repeats_list_query_parameters(httpx_mock): """WQX3 array filters use repeated keys rather than semicolons.""" - with open("tests/data/wqp3_results.txt") as text: + with open("tests/data/wqp3_results.txt", encoding="utf-8") as text: httpx_mock.add_response(method="GET", text=text.read()) get_results( @@ -167,7 +167,7 @@ def test_wqx3_get_results_repeats_list_query_parameters(httpx_mock): ) def test_wqx3_get_results_repeats_iterable_query_parameters(httpx_mock, values_factory): """WQX3 materializes non-list iterables before httpx serialization.""" - with open("tests/data/wqp3_results.txt") as text: + with open("tests/data/wqp3_results.txt", encoding="utf-8") as text: httpx_mock.add_response(method="GET", text=text.read()) _df, md = get_results( @@ -184,7 +184,7 @@ def test_wqx3_get_results_repeats_iterable_query_parameters(httpx_mock, values_f def test_legacy_get_results_preserves_generator_values_in_metadata(httpx_mock): """Legacy serialization must not leave an exhausted metadata iterator.""" - with open("tests/data/wqp_results.txt") as text: + with open("tests/data/wqp_results.txt", encoding="utf-8") as text: httpx_mock.add_response(method="GET", text=text.read()) _df, md = get_results( @@ -200,7 +200,7 @@ def test_legacy_get_results_preserves_generator_values_in_metadata(httpx_mock): def test_wqx3_what_sites_repeats_list_query_parameters(httpx_mock): """The WQX3 serializer also applies to metadata search endpoints.""" - with open("tests/data/wqp_sites.txt") as text: + with open("tests/data/wqp_sites.txt", encoding="utf-8") as text: httpx_mock.add_response(method="GET", text=text.read()) what_sites(legacy=False, siteType=["Stream", "Well"]) @@ -335,7 +335,7 @@ def test_what_query(httpx_mock, func, service, fixture, profile_column): assert profile_column in df.columns # Only get_results post-processes: the shared funnel must hand back each # what_* response exactly as parsed, with no DateTime columns and no sort. - with open(f"tests/data/{fixture}") as text: + with open(f"tests/data/{fixture}", encoding="utf-8") as text: assert_frame_equal(df, _read_wqp_csv(text.read())) _assert_wqp_metadata(md, request_url)