refactor: one advisory mechanism, and the right category for each - #375
Draft
thodson-usgs wants to merge 2 commits into
Draft
refactor: one advisory mechanism, and the right category for each#375thodson-usgs wants to merge 2 commits into
thodson-usgs wants to merge 2 commits into
Conversation
Four spellings of "tell the caller something is going away" had grown up independently: a dated decorator in nwis, an undated kwarg shim in waterdata.utils, an undated module notice in wqp, and one bare warnings.warn with no category at all. Only nwis carried a date, so the horizons could not be audited in one place, and the category was a per-author choice. The category is the part that bites. Two advisories that most needed the same treatment had opposite ones: - **wqp's legacy-format notice was a DeprecationWarning.** ``legacy=True`` is the default on every wqp getter and ``wqp_url`` warns unconditionally, so a downstream project running ``-W error::DeprecationWarning`` -- ordinary CI hygiene -- could not call any wqp getter with default arguments at all. It is also not a deprecation: no name in this package is going away and the caller has nothing to migrate to. The USGS data behind the legacy WQX format simply stopped being updated. - **The NWIS qw endpoint retirement was a bare UserWarning**, so the same filter that over-caught wqp ignored a genuine retirement notice entirely. Both are now ``DataCurrencyWarning`` (a ``UserWarning`` subclass in the exceptions taxonomy, beside ``SkippedItemWarning``): the API is fine, the upstream dataset is not. ``DeprecationWarning`` now means only what it should -- a name in this package is going away, with a replacement and, where one has been published, a date. ``_deprecation.warn_deprecated`` owns the message assembly and ``REMOVALS`` the horizons; nwis and the waterdata kwarg shim both route through it, keeping their distinct granularities (function vs keyword), which is genuine variation rather than duplication. Placed at the layers floor -- it imports nothing first-party. A test pins the behavioural claim: wqp getters remain callable under ``-W error::DeprecationWarning``, and the two categories stay independently filterable so silencing stale data cannot silence a real removal notice. 791 passed, mypy --strict clean, all hooks including import-linter pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS
Code review of this PR found that moving WQP's legacy notice to DataCurrencyWarning broke the code that suppresses it, and that folding `detail` into `replacement` corrupted the kwarg message. - **wqp._legacy_only_url suppressed the wrong category.** It ignores the legacy advisory for endpoints with no WQX3.0 equivalent, because that advisory tells the caller to set `legacy=False` -- which they just did. The suppression named DeprecationWarning, so once the advisory became a DataCurrencyWarning it stopped matching, and `what_sites(legacy=False)` emitted exactly the lie the function exists to prevent. Now pinned by a test, which is what was missing: nothing covered the suppression. - **`detail` is appended again, not interpolated.** Folding it into `replacement` nested it inside a parenthetical mid-sentence, producing a message that claimed both "in a future release" and "on or after 2027-08-09". The date is now passed as `removal=`, so there is one horizon per advisory, and `detail` is a trailing sentence as its docstring always said. - **REMOVALS records horizons this tree actually publishes.** The `wateruse` entry was read by nothing here (it lives on the unmerged configuration branch) while the real published date -- `service=` -> `collection`, 2027-08-09 -- stayed hardcoded in waterdata/cql.py. The table now holds the latter and cql.py reads it, so the horizons can be audited in one place, which was the point. - **DataCurrencyWarning is exported at package top level**, like every other taxonomy name. Callers are meant to filter this category; making them reach into `dataretrieval.exceptions` for it undercut the premise. - **NEWS entry**, including the visibility trade the previous commit did not state: DeprecationWarning is silent by default outside __main__, so this notice now prints for library and notebook callers who never saw it. The entry names the filter to silence it. 793 passed, mypy --strict clean, all hooks pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug this fixes
legacy=Trueis the default on every wqp getter, andwqp_urlwarns unconditionally — as aDeprecationWarning. So a downstream project running-W error::DeprecationWarning, ordinary CI hygiene, cannot call any wqp getter with default arguments:It is also not a deprecation. No name in this package is going away and the caller has nothing to migrate to — the USGS data behind the legacy WQX format simply stopped being updated in March 2024.
Meanwhile the actual NWIS qw endpoint retirement was a bare
warnings.warnwith no category at all, so it arrived as aUserWarningthat the very same filter ignores. The two advisories that most needed the same treatment had opposite ones.What changed
Both are now
DataCurrencyWarning— aUserWarningsubclass in the exceptions taxonomy, besideSkippedItemWarning. The distinction it draws:DeprecationWarning-W error::DeprecationWarningfails your build, correctlyDataCurrencyWarningFour advisory mechanisms collapse to one.
_deprecation.warn_deprecatedowns the message assembly andREMOVALSthe horizons — previously only nwis carried a date, so the commitments could not be audited or bumped in one place.nwis._deprecatedandwaterdata.utils._accept_legacy_kwargsboth route through it while keeping their distinct granularities (function vs keyword), which is genuine variation rather than duplication._deprecationsits at the layers floor; it imports nothing first-party.Testing
-W error::DeprecationWarning, and the two categories stay independently filterable so silencing stale data cannot silence a real removal notice.mypy --strict,ruff,xenon,complexipy,lint-importsall pass.Found by the scan in #372. Independent of #372/#373/#374 — branches off main.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS