docs: copy edits across the ADRs, guides, and glossary - #417
Merged
Merged
Conversation
Copy edits only: say each thing in plain terms throughout the architecture records, user guides, CONTEXT.md, AGENTS.md, CONTRIBUTING.md, README, and NEWS. No decision, glossary term, or code changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Vb9Y5QQdoHTac4cgvTexc
Copy edits only, continuing the previous commit: plain terms for Retry-After handling, the no-progress budget, host acceptance of the API key, and a few remaining figures of speech in the ADRs, glossary, and guides. No decision, glossary term, or code changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Vb9Y5QQdoHTac4cgvTexc
thodson-usgs
marked this pull request as ready for review
September 6, 2026 22:43
thodson-usgs
added a commit
that referenced
this pull request
Sep 9, 2026
The previous round (#417) covered the ADRs, guides, and glossary. This one extends the same copy editing to every .py file -- module and function docstrings, inline comments, and message strings -- and makes a further pass over the prose #417 touched, so the source and the documentation use the same words for the same things. Figurative phrasing is replaced with the literal statement: - settings, not dials or knobs; parameters, not switches - "time without data" for the stall condition, not silence - "server-specified" for a Retry-After wait, not sanctioned or named - "is raised as", not surfaces; "is emitted", not fires - "without an error" or "undetected", not silently or quietly - "is defined in", not lives in; "run" a fan-out, not drive one - "takes precedence" and "outranks", not wins or beats - literal verbs where code was said to know, want, ask, answer, or trust - emphasis adverbs dropped where they carried no fact The glossary's terms are unchanged: leaf, facade, seam, fan-out, chunk, page walk, dialect, rung and ladder, no-progress budget, and the rest of CONTEXT.md. Test function names, fixture data, and quoted upstream text are untouched. The show_configuration sample output is duplicated in three places -- a docstring, the printed string, and the configuration guide -- and a test compares them, so all three were edited together. No behavior change. An AST comparison over all 81 changed Python files confirms only docstrings, comments, and message text differ; ruff, mypy --strict, and the offline suite pass. Claude-Session: https://claude.ai/code/session_01N1Ap36zDGnifU8GvjnFowm Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
thodson-usgs
added a commit
to thodson-usgs/dataretrieval-python
that referenced
this pull request
Sep 9, 2026
Two conflicts, both from main's copy-edit passes (DOI-USGS#417, DOI-USGS#418) landing on lines this branch had already rewritten: - NEWS.md: main added the 09/01 survey announcement at the top and reworded the 08/27 peaks entry; this branch added the 08/30 ratings entry. Kept all three, newest first, with main's wording of 08/27. - tests/nwis_test.py: main copy-edited the docstring of test_named_replacement_exists_in_waterdata, which this branch had replaced with the stronger test_named_replacement_resolves (ARG001 -- the old test never used its `name` parameter). Kept the replacement and carried main's edits into its docstring. Gates on the merged tree: ruff check + ruff format --check clean at the pinned 0.16.5, mypy --strict clean over 60 files, 1153 tests pass, coverage 98.93%, xenon / complexipy / import-linter (8/8) pass.
thodson-usgs
added a commit
to thodson-usgs/dataretrieval-python
that referenced
this pull request
Sep 9, 2026
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 DOI-USGS#344 -- the suite went 1154 -> 1153 and nothing flagged it, since a missing test cannot fail. - 36 lines of copy edits from DOI-USGS#417 and DOI-USGS#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/``.
thodson-usgs
added a commit
that referenced
this pull request
Sep 14, 2026
#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>
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.
Copy edits only. The ADRs, architecture overview, user guides,
CONTEXT.md,AGENTS.md,CONTRIBUTING.md, README, and NEWS now say each thing in plain terms. No decision, glossary term, or code changes. Sphinx parses clean and the architecture tests pass.🤖 Generated with Claude Code