Repository navigation
Commit 75e56ab
lint(ruff): select the checks no other gate covers, and pin the linter (#398)
* fix(waterdata): write downloaded ratings as UTF-8
`get_ratings(..., file_path=...)` wrote each rating with `open(..., "w")`
and no encoding, so the bytes on disk depended on the writing machine's
locale rather than on what the service sent.
On a non-UTF-8 locale an unrepresentable character raises
`UnicodeEncodeError`, which is a `ValueError`, which `_download_all`'s
per-feature handler downgrades to a `SkippedRatingWarning` -- so the rating
disappears from the returned dict rather than failing loudly, which is the
opposite of what the function's own docstring promises. `newline=""`
disables the translation that would otherwise rewrite the RDB's line
endings on Windows.
The regression test asserts the saved file equals the response body byte for
byte, using a character absent from cp1252, so it fails on the Windows leg
without the fix and cannot fail on Linux or macOS.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T6MVcko4gh68LieGWxUnrR
* fix(waterdata): move the ratings write off the event loop
The same `open()` was a blocking call inside `async def _fetch_rating`, which
runs dozens-concurrent under a fan-out drive -- so every rating write stalled
every other in-flight download for its duration.
The write is now a three-line `_write_rating` dispatched through
`anyio.to_thread.run_sync`. Nothing in the suite would have caught this: the
tests await mocked transports, so a stalled loop still passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T6MVcko4gh68LieGWxUnrR
* fix(nwis): stop setting the index on the caller's frame in place
`format_response` set the index with `inplace=True` on the frame it was
handed while already returning a new object -- and already left the caller's
frame untouched on the `peaks` and GeoDataFrame paths, so the in-place branch
was the odd one out.
Callers using the return value, which is the only documented use, see no
difference. `pandas` is also phasing `inplace=` out.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T6MVcko4gh68LieGWxUnrR
* fix(nwis): warn for the get_record options whose services are defunct
`get_record`'s `wide_format`, `datetime_index` and `state` have read nothing
since `qwdata`, `gwlevels` and `water_use` were retired -- their only readers
went with those branches -- but all three stayed in the signature and in the
published docstring. `get_record(sites=..., state="OH")` therefore reads as a
state filter while doing nothing at all.
Verified against the tree before `491eb5c3` ("remove usage of qwdata"), where
`get_record` passed `wide_format` and `datetime_index` to `get_qwdata`,
`datetime_index` to `get_gwlevels`, and `state` to `get_water_use` -- the four
readers (`if wide_format:`, two `if datetime_index is True:`, and
`if state is not None:`) that went with those functions. Nothing in the
package reads any of the three today.
They now advise through `_deprecation.warn_deprecated`, naming a live
replacement each, and are still accepted and still ignored. Naming one at its
declared default is silent: the caller is asking for exactly what the dead
default already gave them, so only a value the option cannot honour is worth
a warning. A test pins that, and with it the agreement between the table's
"unset" value and `get_record`'s declared default that the distinction rests
on.
They are not raised on and not deleted:
- A defunct *service* cannot return the data asked for; `get_record(
service="dv", wide_format=False)` returns exactly the right data, and only
the knob is dead. Raising would retire a documented parameter of a
Production/Stable getter ~20 months before `REMOVALS["nwis"]` with no
warned release in between.
- Deleting `state` from the signature would let it fall through `**kwargs` to
`query_waterservices` and onto the wire as a parameter NWIS does not
define. Confirmed against the live code path: an unknown *string* kwarg
reaches the query string, while an unknown *bool* is dropped, because
`to_str` returns `None` for a non-iterable scalar. So the argument for
keeping the parameter holds for `state` alone.
A call naming all three emits four `DeprecationWarning`s: one from
`@_deprecated` plus one per option. They are not duplicates -- each names a
distinct subject and a distinct replacement -- but `_warn_defunct_record_options`
raises through `warn_deprecated` directly and so does not pass through
`_deprecated`'s re-entrancy guard. The count is pinned by a test, as is the
attribution the hand-counted `stacklevel=4` encodes.
The replacement tripwire is now derived from the deprecation tables
themselves rather than from a hand-kept list, and checks the keywords a
message names as well as the function.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T6MVcko4gh68LieGWxUnrR
* lint(ruff): fix what the checks in the next commit found
Everything the newly selected rules reported that is not a bug in its own
right, kept apart from the config change so the config commit is only config:
- `D100`: `dataretrieval.codes` and `dataretrieval.waterdata.types` are
rendered by `automodule ... :members:`, so a missing module docstring ships
to the docs site as a bare signature. `setup.py` and `docs/source/conf.py`
get one-liners.
- `ARG001`: `_display_api_key` and `_display_progress` never read `adapter`;
it is there because the display registry calls every renderer with the same
signature, so the name is prefixed rather than removed.
- `RUF100`: four `# noqa: BLE001` directives sat on handlers that re-raise and
never needed one. Four more carried their reason after a dash or in
parentheses -- ruff reads the rest of the line as the directive's
description and would delete a co-located `# pylint: disable=` with it -- so
they are respelled with a second `#`.
- `PLW1514`: the test helpers that read fixture files now name an encoding.
- `PD002`: `_parse_parameter_record` returns the renamed frame rather than
renaming in place; the frame is local, so nothing outside changes.
- `PD003`: `isnull` -> `isna`.
- `DTZ007`: `ogc/dates.py`'s `_parse_datetime` is correct by design --
`_DATETIME_FORMATS` deliberately carries both the `%z` and the bare forms,
and the docstring promises tz-awareness only for an input that carried an
offset. It takes a directive naming the reason rather than a "fix".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T6MVcko4gh68LieGWxUnrR
* lint(ruff): select the checks no other gate covers, and pin the linter
Config only; the findings are fixed in the commits before this one.
## The rules
- `PLW1514` -- mypy does not model encodings, and a locale-dependent write
misbehaves only on the Windows leg, so a green Linux run proves nothing.
Found the silent rating loss.
- `ARG001` -- notices when retiring a legacy getter strands a documented
keyword. Found the three inert `get_record` options.
- `ASYNC` -- the suite awaits mocked transports, so a stalled event loop still
passes. Found the blocking ratings write.
- `D100`-`D104` -- `automodule ... :members:` renders a docstring-less public
symbol as a bare signature.
- `RUF100` -- nothing was watching the suppressions. Six `# noqa: BLE001`
directives sat in the tree suppressing a rule nobody had selected.
- `BLE001` -- a blind `except Exception` skips ADR 0004's transient-versus-
fatal judgement. Already half-adopted, via those six directives.
- `PD`, `DTZ` -- `inplace=` is on pandas' way out, in a pandas library; a
naive `datetime` in a library about time series.
- `C4` `LOG` `G` `T20` `FA` `NPY` -- free ratchets at today's setting.
The suppression budget goes down, not up: four directives removed, one added.
## The flow
- The version was pinned in three places and honoured in none: the pre-commit
rev, CI and the `test` extra all said 0.16.1, while the machine that wrote
this had 0.16.5 on `$PATH` and 0.15.12 in its venv -- `ruff format --check`
disagreed by 28 files between them. It now lives in two: a `[lint]` extra
that CI installs the way the `[metrics]` job already installs its own, and
the `ruff-pre-commit` rev. The `test` extra reuses `dataretrieval[lint]`, as
it already reuses `dataretrieval[type-check]`.
- Pinned at 0.16.5, the current release.
- `preview = true` enabled 52 rules to get one. `explicit-preview-rules` keeps
the rest off: that count was 50 at 0.16.1 and is 52 at 0.16.5, four patch
releases apart, and each would have arrived as a CI failure nobody selected.
- `E501` was selected redundantly (already inside `E`) under a comment naming
a `line-length` setting that did not exist. The value is now set explicitly,
where it visibly governs both the checker and the formatter.
Unrelated but found the same way: the mypy `anyio` override is dropped. Its
rationale was a 3.9 target that a `match` statement would fail to parse, and
`python_version` has been 3.10 since 1.2.0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T6MVcko4gh68LieGWxUnrR
* docs(news): record the two ratings fixes
The `nwis.get_record` option deprecations and the `format_response` change
are not called out: `nwis` is deprecated itself, and the entry covers
interface changes and bug fixes to active modules.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T6MVcko4gh68LieGWxUnrR
* perf(nwis): keep the caller's frame without duplicating it
Dropping `inplace=` from `format_response`'s `set_index` stopped the function
from mutating its argument, but it also made the function copy the data.
pandas resolves a non-inplace `set_index` to `self.copy(deep=None)`, and
`BlockManager.copy` turns `deep=None` into a full deep copy whenever
copy-on-write is off -- the default for all of pandas 2.x, which is what a
Python 3.10 install gets, since pandas 3.0 requires 3.11.
A shallow copy carries the same guarantee: the index lands on our frame and
the columns stay shared. `set_index` then runs inplace on that copy, which is
what the `noqa` records.
The rest of the gap is `_localize_datetime_index`. `DataFrame.tz_localize`
relabels one axis by duplicating every column, a copy main paid too.
Retagging the index alone is equivalent -- asserted frame-equal against the
old call on both the multi-index and single-index branches -- and costs
nothing.
Peak RSS for one call, measured in a fresh process on pandas 2.2.3:
main inplace dropped here
2M rows, 1 site 108.0 123.3 46.2 MiB
2M rows, 8 sites 250.0 394.6 318.3 MiB
1M rows, 20 cols, 8 sites 389.2 456.6 288.7 MiB
The narrow multi-site frame stays above main. Building a two-level MultiIndex
while the source columns stay materialized is the price of leaving the
caller's frame intact; only mutating the argument avoids it. The other two
shapes fall below main because the tz_localize copy is gone.
CPU improves alongside: 355 -> 319 ms on pandas 2.2.3 and 227 -> 215 ms on
3.0.5, for 2M rows across 8 sites. pandas 3.0.5 memory is unchanged
throughout -- copy-on-write already made both copies shallow, which is why
the suite cannot see any of this: CI runs 3.13 and 3.14.
So the test asserts the property that does hold everywhere -- the argument
keeps its columns and its index -- which fails against the in-place version
on both branches.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVhHGSDkVxWzeUMmHn6oAT
* fix(nwis): stop the peaks path writing into the caller's frame
A review of the previous commit found that the invariant its test states is
broader than what the code delivers. `preformat_peaks_response` derives
`datetime` from `peak_dt` with a plain assignment, so
`format_response(df, service="peaks")` still added a column to the frame it
was handed: the shallow copy the previous commit introduced sits downstream
of that call. `preformat_peaks_response` is public in its own right, so a
caller reaches the mutation without going through `format_response` at all.
It now derives the column on its own frame, and the test class covers the
peaks arm alongside the plain ones.
That also settles a claim in e773171, which said the function already left
the caller's frame untouched on the peaks and GeoDataFrame paths. It held for
GeoDataFrame only. It holds for both now.
`_localize_datetime_index` loses its frame wrapper. Its one caller hands it a
frame it has just shallow-copied, so the second copy -- and the identity guard
that existed to avoid it -- protected a caller that no longer exists. As
`_localized_datetime_index` it takes an index and returns one, and the caller
assigns. Frame-equal to the previous form on pandas 2.2.3 and 3.0.5 across
single-site, multi-site, already-aware and no-datetime frames; peak RSS is
unchanged at 46.2 / 317.3 / 288.6 MiB on the three shapes the previous commit
measured.
The `datetime_index` entry told a caller to reach for
`waterdata.get_continuous` or `get_daily` "which index by datetime". Neither
does: no getter under `waterdata/` sets an index, and the suite pins `time` as
an ordinary column. The recommendation is right, the reason given for it was
not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVhHGSDkVxWzeUMmHn6oAT
* fix(merge): restore what main brought to tests/nwis_test.py
The merge resolved that file with ``git checkout --ours``, which takes the
whole ours-side blob rather than the one conflicted region, so everything
main brought to the file was reverted:
- ``TestReadRdb::test_malformed_peaks_frame_still_raises``, the regression
test from #344 -- the suite went 1154 -> 1153 and nothing flagged it,
since a missing test cannot fail.
- 36 lines of copy edits from #417 and #418, the passes that replaced
figurative wording with literal ("has no opinion about it" -> "does not
record it", "borrowing" -> "importing").
Redone as a three-way merge of the file with only the conflicted region
resolved by hand. That region is main's copy edit of
``test_named_replacement_exists_in_waterdata``, a test this branch had
already replaced with the stronger ``test_named_replacement_resolves``
(ARG001: the old one never read its ``name`` parameter). The replacement
stands, with main's edits carried into its docstring -- the "Tripwire:"
prefix and the contraction dropped, matching the pass that removed the
only other instance of each in ``tests/``.
* docs: clarify deprecation guidance and review terminology
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>1 parent 0d345a7 commit 75e56ab
24 files changed
Lines changed: 391 additions & 99 deletions
File tree
- .github/workflows
- dataretrieval
- codes
- ogc
- transport
- waterdata
- docs/source
- tests
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
23 | | - | |
24 | | - | |
| 23 | + | |
25 | 24 | | |
26 | 25 | | |
27 | 26 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
33 | | - | |
| 33 | + | |
34 | 34 | | |
35 | 35 | | |
36 | 36 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
70 | 70 | | |
71 | 71 | | |
72 | 72 | | |
73 | | - | |
74 | | - | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
75 | 76 | | |
76 | 77 | | |
77 | 78 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
| 6 | + | |
5 | 7 | | |
6 | 8 | | |
7 | 9 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
1 | 7 | | |
2 | 8 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
804 | 804 | | |
805 | 805 | | |
806 | 806 | | |
807 | | - | |
| 807 | + | |
808 | 808 | | |
809 | 809 | | |
810 | 810 | | |
| |||
814 | 814 | | |
815 | 815 | | |
816 | 816 | | |
817 | | - | |
| 817 | + | |
818 | 818 | | |
819 | 819 | | |
820 | 820 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
148 | 148 | | |
149 | 149 | | |
150 | 150 | | |
151 | | - | |
| 151 | + | |
152 | 152 | | |
153 | | - | |
| 153 | + | |
154 | 154 | | |
155 | | - | |
156 | | - | |
157 | | - | |
158 | | - | |
159 | | - | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
160 | 160 | | |
161 | 161 | | |
162 | 162 | | |
| |||
198 | 198 | | |
199 | 199 | | |
200 | 200 | | |
201 | | - | |
| 201 | + | |
202 | 202 | | |
203 | | - | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
204 | 210 | | |
205 | | - | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
206 | 214 | | |
207 | 215 | | |
208 | 216 | | |
| |||
238 | 246 | | |
239 | 247 | | |
240 | 248 | | |
| 249 | + | |
| 250 | + | |
241 | 251 | | |
242 | 252 | | |
243 | 253 | | |
| |||
897 | 907 | | |
898 | 908 | | |
899 | 909 | | |
| 910 | + | |
| 911 | + | |
| 912 | + | |
| 913 | + | |
| 914 | + | |
| 915 | + | |
| 916 | + | |
| 917 | + | |
| 918 | + | |
| 919 | + | |
| 920 | + | |
| 921 | + | |
| 922 | + | |
| 923 | + | |
| 924 | + | |
| 925 | + | |
| 926 | + | |
| 927 | + | |
| 928 | + | |
| 929 | + | |
| 930 | + | |
| 931 | + | |
| 932 | + | |
| 933 | + | |
| 934 | + | |
| 935 | + | |
| 936 | + | |
| 937 | + | |
| 938 | + | |
| 939 | + | |
| 940 | + | |
| 941 | + | |
| 942 | + | |
| 943 | + | |
| 944 | + | |
| 945 | + | |
| 946 | + | |
| 947 | + | |
900 | 948 | | |
901 | 949 | | |
902 | 950 | | |
| |||
929 | 977 | | |
930 | 978 | | |
931 | 979 | | |
932 | | - | |
933 | | - | |
| 980 | + | |
| 981 | + | |
| 982 | + | |
| 983 | + | |
| 984 | + | |
934 | 985 | | |
935 | | - | |
| 986 | + | |
| 987 | + | |
| 988 | + | |
| 989 | + | |
| 990 | + | |
| 991 | + | |
| 992 | + | |
936 | 993 | | |
937 | | - | |
| 994 | + | |
| 995 | + | |
| 996 | + | |
| 997 | + | |
| 998 | + | |
938 | 999 | | |
939 | 1000 | | |
940 | 1001 | | |
| |||
1013 | 1074 | | |
1014 | 1075 | | |
1015 | 1076 | | |
| 1077 | + | |
| 1078 | + | |
| 1079 | + | |
| 1080 | + | |
1016 | 1081 | | |
1017 | 1082 | | |
1018 | 1083 | | |
| |||
1099 | 1164 | | |
1100 | 1165 | | |
1101 | 1166 | | |
1102 | | - | |
| 1167 | + | |
1103 | 1168 | | |
1104 | 1169 | | |
1105 | 1170 | | |
1106 | 1171 | | |
1107 | | - | |
1108 | | - | |
| 1172 | + | |
1109 | 1173 | | |
1110 | | - | |
1111 | 1174 | | |
1112 | 1175 | | |
1113 | 1176 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
53 | 53 | | |
54 | 54 | | |
55 | 55 | | |
56 | | - | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
57 | 59 | | |
58 | 60 | | |
59 | 61 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
115 | 115 | | |
116 | 116 | | |
117 | 117 | | |
118 | | - | |
| 118 | + | |
119 | 119 | | |
120 | 120 | | |
121 | 121 | | |
| |||
140 | 140 | | |
141 | 141 | | |
142 | 142 | | |
143 | | - | |
| 143 | + | |
144 | 144 | | |
145 | 145 | | |
146 | 146 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
289 | 289 | | |
290 | 290 | | |
291 | 291 | | |
292 | | - | |
| 292 | + | |
293 | 293 | | |
294 | 294 | | |
295 | 295 | | |
| |||
311 | 311 | | |
312 | 312 | | |
313 | 313 | | |
314 | | - | |
| 314 | + | |
315 | 315 | | |
316 | 316 | | |
317 | 317 | | |
| |||
0 commit comments