Skip to content

fix(waterdata): keep sublocation_identifier from shifting positional arguments - #447

Merged
thodson-usgs merged 1 commit into
DOI-USGS:mainfrom
thodson-usgs:fix/sublocation-review-followups
Oct 12, 2026
Merged

thodson-usgs merged 1 commit into
DOI-USGS:mainfrom
thodson-usgs:fix/sublocation-review-followups

Conversation

@thodson-usgs

Copy link
Copy Markdown
Collaborator

Follow-ups from a review of #436 after it merged. #436 is not in a release yet (v1.4.0 predates it), so moving the parameter breaks no released caller.

  • Signature order. fix(waterdata): accept field-measurements sublocation_identifier #436 inserted sublocation_identifier mid-signature in get_field_measurements (before skip_geometry) and get_field_measurements_metadata (before properties). Every later positional argument shifted, so a positional skip_geometry or properties would have been sent as a sublocation filter. The parameter now comes last, before **queryables. get_continuous's method_category (fix(waterdata): accept the continuous method_category queryable #423) has the same problem but shipped in v1.4.0, so it is left alone.
  • Test. The shared fixture's site (USGS-05427718) records no sublocation, so the test only checked that a hand-added null column exists. It now uses real features captured from USGS-05435943, one each for Backup, HWM and Primary (new tests/data/waterdata_sublocation_fixtures.json), and asserts those values.
  • Docs. get_time_series_metadata and get_combined_metadata listed sublocation_identifier with no description; it is described now, with free-text values observed there (YSI DOWN, 30 CM). The field-measurement examples now use observed values; UPSTREAM was not among them.
  • NEWS. The entry is re-dated to its land date (10/07), its hard wraps are removed, and it notes that the field already worked through **queryables.

Not done: an offline test comparing each getter's parameters with tests/data/waterdata_queryables.json. Queryables are filters and not all are meant to be named parameters (monitoring-locations has about 240), so the existing /schema columns check from #424 is the better place for this.

Overlap with #446

#446 rewrites the list-case assertion in test_sublocation_identifier_is_sent_as_a_filter, from a comma join to a CQL2 in. This PR changes the adjacent lines, so whichever PR merges second needs a trivial rebase.

Verification

  • 1,254 offline tests pass, coverage 99.02%; ruff, mypy, xenon, complexipy and import-linter are clean.
  • The live queryables snapshot test passes (11/11).
  • Live: get_field_measurements(monitoring_location_id="USGS-05435943", sublocation_identifier="HWM") returns 2 rows, and get_field_measurements_metadata(..., sublocation_identifier="Backup") returns 1.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HBt3Ee7eneKtdKQWq2T3YK

…arguments

Follow-ups from review of DOI-USGS#436, which is unreleased:

- Move sublocation_identifier to the end of the get_field_measurements and
  get_field_measurements_metadata signatures. Inserted mid-signature, it
  shifted every later positional argument: a positional skip_geometry or
  properties would have been sent as a sublocation filter.
- Test against real features from USGS-05435943 (Backup, HWM, Primary)
  and assert the values; the shared fixture's site records no sublocation,
  so the old test only checked a hand-added null column.
- Describe the parameter in get_time_series_metadata and
  get_combined_metadata, and use observed values in the examples.
- Date the NEWS entry by its land date (10/07), unwrap it, and note that
  the field already worked through **queryables.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HBt3Ee7eneKtdKQWq2T3YK
@thodson-usgs
thodson-usgs marked this pull request as ready for review October 12, 2026 12:42
@thodson-usgs
thodson-usgs merged commit 33d76c7 into DOI-USGS:main Oct 12, 2026
11 checks passed
@thodson-usgs
thodson-usgs deleted the fix/sublocation-review-followups branch October 12, 2026 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant