From e1901c358691564df1a48fe83f15cd93d793153f Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Sun, 30 Aug 2026 16:46:24 -0500 Subject: [PATCH] chore: keep what the pylint evaluation found, without keeping pylint Pylint was evaluated as a periodic second opinion and not adopted: 87% of what it reports are checks ruff also implements, and its `unused-argument` check cannot see through the `locals()` forwarding the collection getters use. CONTRIBUTING records the measurement so nobody re-derives it, and the one check with no other home -- an `except` tuple whose members shadow one another, which ruff's B014 misses because it knows alias pairs rather than subclass relationships -- is written down as a one-line command instead of a dependency. Four things it found that are worth keeping: - `nwis._parse_json_or_raise` caught `(ValueError, JSONDecodeError)`, and `JSONDecodeError` subclasses `ValueError`, so the tuple advertised a distinction that cannot exist. - `tests/waterdata_progress_test.py` held an `async def parse_response` left from an earlier revision; the test passes `parse_sync`. - mypy's `possibly-undefined` catches a name bound on only some paths -- a runtime `NameError` on the branch nobody tested. mypy ships it but leaves it off even under `strict`, and nothing else in the stack models it. The package is clean today, so it lands as a ratchet. - The wheel smoke test named six imports and reached most of the tree by luck of what those names pull in. It now walks every module the installed wheel ships, and asserts the walk saw all of them -- `walk_packages` skips the subtree under a package it cannot import and says nothing when it does. It also pins the optional-dependency guard: without geopandas, importing `nldi` must fail with the message that says how to fix it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T6MVcko4gh68LieGWxUnrR --- .github/workflows/python-package.yml | 32 ++++++++++++++++++++++++++++ CONTRIBUTING.md | 23 ++++++++++++++++++++ dataretrieval/nwis.py | 3 +-- pyproject.toml | 7 ++++++ tests/waterdata_progress_test.py | 7 ------ 5 files changed, 63 insertions(+), 9 deletions(-) diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index 807532c8..69b368a1 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -107,6 +107,38 @@ jobs: assert ngwmn.get_sites assert wateruse.get_wateruse assert engine.get_ogc_data + + # The named imports above reach most of the tree but not all of it, + # and a dependency dropped from [project.dependencies] is only + # visible where something imports it. Walk every module instead, so + # the gap cannot depend on which names this heredoc happens to list. + import importlib + import pkgutil + + root = Path(dataretrieval.__file__).parent + shipped = { + ".".join(("dataretrieval", *p.relative_to(root).with_suffix("").parts)) + .removesuffix(".__init__") + for p in root.rglob("*.py") + } - {"dataretrieval"} + walked = set() + for info in pkgutil.walk_packages(dataretrieval.__path__, "dataretrieval."): + walked.add(info.name) + if info.name != "dataretrieval.nldi": # asserted separately below + importlib.import_module(info.name) + # walk_packages skips the subtree under a package it cannot import, + # and says nothing when it does. + assert walked == shipped, shipped ^ walked + + # The optional geospatial dependency stays optional: without + # geopandas installed, importing nldi must fail with the message that + # says how to fix it, not with a bare ModuleNotFoundError. + try: + import dataretrieval.nldi + except ImportError as exc: + assert "geopandas" in str(exc), exc + else: + raise AssertionError("nldi imported without geopandas installed") PY type-check: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3b5af121..11709601 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -236,6 +236,29 @@ as tools (`analyze_code`, `detect_clones`, `find_dead_code`, `get_health_score`, and others). Registering it with an MCP-capable assistant is a personal workflow choice, so this repository does not configure one. +`pylint` was evaluated as a periodic second opinion and deliberately not +adopted. Over `dataretrieval/`, pylint 4.0.8 with its default checks reports +635 findings; all but fourteen are checks ruff also implements, and 419 are a +single false positive -- its `unused-argument` cannot see through the +`locals()` forwarding the collection getters use, so every documented keyword +argument they accept looks unused. The fourteen ruff has no rule for are size +metrics, duplication, and one unused wildcard re-export: duplication is +`pyscn`'s job, the size metrics grade a class-oriented design this package does +not have, and the wildcard is a deprecated alias module re-exporting its +replacement on purpose. The capability ruff lacks is import resolution, and a +missing dependency is already caught by the wheel smoke test in +`python-package.yml`, which imports every module the wheel ships from an +installed wheel outside the checkout. One check has no other home -- an +`except` tuple whose members shadow one another, which ruff's B014 does not +catch because it knows alias pairs rather than subclass relationships. If you +want that second opinion, it costs one command and no configuration: + +```bash +uvx --with pandas --with httpx --with anyio pylint -j0 --disable=all \ + --enable=bad-except-order,overlapping-except \ + --load-plugins=pylint.extensions.overlapping_exceptions dataretrieval tests +``` + For documentation changes, install `.[doc,nldi]` and run `make html` from `docs/`. The broader `make docs` target also runs doctests and network-dependent link checking. diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 4ef7b01a..948396ce 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -10,7 +10,6 @@ import threading import warnings from collections.abc import Callable -from json import JSONDecodeError from typing import Any, NoReturn, TypeVar, cast import httpx @@ -128,7 +127,7 @@ def _parse_json_or_raise(response: httpx.Response) -> pd.DataFrame: """Parse a JSON NWIS response, raising a helpful error on HTML responses.""" try: return _read_json(response.json()) - except (ValueError, JSONDecodeError) as e: + except ValueError as e: text_lower = response.text.lower() content_type = response.headers.get("Content-Type", "").lower() if ( diff --git a/pyproject.toml b/pyproject.toml index 4da15c3d..24bfaabb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -163,6 +163,13 @@ python_version = "3.10" # the project's minimum supported version files = ["dataretrieval"] strict = true ignore_missing_imports = true +# ``possibly-undefined`` catches a name that is bound on only some paths -- the +# ``if`` with no ``else`` whose variable is read afterwards, which fails at +# runtime with ``NameError`` and only on the branch nobody tested. mypy ships +# the check but leaves it off even under ``strict``, and neither ruff nor the +# suite models it. The package is clean today, so this is a ratchet at its +# tightest current setting rather than a request for work. +enable_error_code = ["possibly-undefined"] # anyio ships ``py.typed``, so mypy follows into its source — which uses # ``match`` statements (3.10+). Under our ``python_version = "3.9"`` target that diff --git a/tests/waterdata_progress_test.py b/tests/waterdata_progress_test.py index b05ba8f2..d4cc9b27 100644 --- a/tests/waterdata_progress_test.py +++ b/tests/waterdata_progress_test.py @@ -473,13 +473,6 @@ def test_paginate_reports_pages_through_active_reporter(monkeypatch): ) resp2 = _resp([{"id": "2", "properties": {"v": "b"}}], rate_remaining="4998") - async def parse_response(resp): - body = resp.json() - nxt = next( - (link["href"] for link in body["links"] if link["rel"] == "next"), None - ) - return mock.MagicMock(empty=False, __len__=lambda self: 1), nxt - # parse_response is sync (like the page parsers). def parse_sync(resp): body = resp.json()