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()