Fix WQP_Metadata.site_info: bind as a real property and forward kwargs by thodson-usgs · Pull Request #249 · DOI-USGS/dataretrieval-python

thodson-usgs · 2026-05-03T17:56:32Z

Summary

WQP_Metadata.site_info was defined inside __init__ as a nested @property-decorated function that was never bound to the class. The decorator returned a property descriptor that was immediately discarded when __init__ returned, so md.site_info always fell through to BaseMetadata.site_info and raised NotImplementedError. The closure also used parameters[...] (the captured **parameters kwargs) for the lookup but self._parameters for the in check — an internal inconsistency.

Compounding the issue, every helper called WQP_Metadata(response) with no kwargs, so self._parameters was always {} even after fixing the closure.

This PR:

Moves site_info to a real @property on the class.
Looks up siteid first (WQP-native; the previous sites / site / site_no keys reflected an NWIS copy-paste and never matched a WQP query) while keeping the legacy aliases as a fallback.
Threads **kwargs through WQP_Metadata(response, **kwargs) from all nine call sites so the property has the parameters it needs.
Corrects the docstring: the attribute is comment (singular, inherited from BaseMetadata) and query_time is a datetime.timedelta (was datetme.timedelta).

Minimal reproducible example

Pre-fix bug

import dataretrieval.wqp as wqp

df, md = wqp.what_sites(legacy=True, siteid="USGS-01646500")
md.site_info
# NotImplementedError: BaseMetadata does not implement site_info

The pre-fix shape that causes this:

class WQP_Metadata(BaseMetadata):
    def __init__(self, response, **parameters):
        self._response = response
        self._parameters = parameters
        @property                 # decorator runs here
        def site_info(self):      # but the result is a local variable —
            ...                   # it's never attached to the class.

So md.site_info always resolves to BaseMetadata.site_info, which raises.

Post-fix, live API

import dataretrieval.wqp as wqp

df, md = wqp.what_sites(legacy=True, siteid="USGS-01646500")
print(len(df))                      # 1
result = md.site_info
print(type(result).__name__)        # tuple
df2, md2 = result
print(len(df2))                     # 1

Test plan

New regression tests in tests/wqp_test.py: site_info resolves and returns a (DataFrame, WQP_Metadata) tuple when siteid was provided; returns None when no site filter was supplied.
All tests/wqp_test.py (15 tests) pass.
Full local test suite passes.
Live verification against the WQP: md.site_info now returns the expected tuple.

Related PRs

Other open PRs in this bug-review series that touch dataretrieval/wqp.py (different functions, no functional conflicts):

Fix get_results: preserve user-supplied WQX3.0 dataProfile #250 — preserve user-supplied WQX3.0 dataProfile in get_results.
Polish wqp.py: silence prints, fix exception shape, fix typos #256 — polish (warnings instead of print, exception shape, typos, helper extraction).

`site_info` was defined inside `WQP_Metadata.__init__` as a nested `@property`-decorated function that was never bound to the class. The decorator returned a property descriptor that was immediately discarded when `__init__` returned, so `md.site_info` always fell through to `BaseMetadata.site_info` and raised `NotImplementedError`. The closure also used `parameters[...]` (the captured `**parameters` kwargs) for the lookup but `self._parameters` for the `in` check — inconsistent. Compounding the issue, every helper called `WQP_Metadata(response)` with no kwargs, so `self._parameters` was always `{}` even after fixing the closure. Move `site_info` to a real property on the class, look up `siteid` first (WQP-native; the previous `sites`/`site`/`site_no` keys reflected an NWIS copy-paste and never matched a WQP query), and thread `**kwargs` through `WQP_Metadata(response, **kwargs)` from all nine call sites so the property has the parameters it needs. Also corrected the docstring: the attribute is `comment` (not `comments`, inherited from `BaseMetadata`), and the `query_time` type is `datetime.timedelta` (not `datetme.timedelta`). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Folds the special-cased `siteid` branch into the loop over candidate keys; semantics are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Copilot

Pull request overview

Fixes WQP_Metadata.site_info so it’s a real, class-bound property and ensures request parameters are carried into the metadata object, enabling md.site_info to resolve station metadata instead of falling through to BaseMetadata.site_info.

Changes:

Convert WQP_Metadata.site_info from an unbound __init__-local closure into a proper @property on the class and make parameter lookup consistent.
Forward **kwargs into WQP_Metadata(response, **kwargs) from get_results() and all WQP “what_*” helper call sites so metadata can reference the original query parameters.
Add regression tests ensuring md.site_info resolves when siteid is provided and returns None when no site filter exists.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`dataretrieval/wqp.py`	Passes query kwargs into `WQP_Metadata` and redefines `site_info` as a real property to enable station lookups.
`tests/wqp_test.py`	Adds regression coverage for `WQP_Metadata.site_info` resolution and the no-site-filter case.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    @property
+    def site_info(self):
+        for key in ("siteid", "sites", "site", "site_no"):
+            if key in self._parameters:
+                return what_sites(siteid=self._parameters[key])
+        return None


+def test_wqp_metadata_site_info_property_resolves(requests_mock):
+    """`site_info` must be a real property bound to the class.
+
+    Regression: previously `site_info` was defined as a closure inside
+    `__init__`, so `md.site_info` fell through to `BaseMetadata.site_info`
+    and raised `NotImplementedError`. Also verify the parameters are
+    threaded through from `get_results`.
+    """


Per copilot review on PR DOI-USGS#249. The site_info property previously called what_sites() with default legacy=True/ssl_check=True, so a WQX3.0 get_results(legacy=False, ...) call produced a legacy Station lookup. WQP_Metadata now persists the originating legacy/ssl_check on the instance and forwards them when site_info is accessed. All nine WQP_Metadata call sites now thread these through. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

…-info # Conflicts: # tests/wqp_test.py

Per /simplify review on PR DOI-USGS#249: - Switch `site_info` from `@property` to `functools.cached_property`. Repeated access of `md.site_info` (common in notebooks) now reuses the resolved tuple instead of issuing a fresh `what_sites` HTTP call on every read. - Add a one-line comment explaining the legacy-alias -> `siteid` coercion: whichever of `siteid`/`sites`/`site`/`site_no` matched, the value is passed as `siteid` (what_sites' native WQP arg). Also confirmed the docstring's `comments` -> `comment` rename matches `BaseMetadata.comment` (singular) defined at `dataretrieval/utils.py:130`. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

WQP_Metadata is not strictly a snapshot — a long-lived `md` reference (paginated workflow, long-running script) would see stale site metadata if `site_info` cached its result. Plain `@property` matches user expectation that an attribute named `site_info` reflects the current state, and the cost of one extra HTTP call on re-read is acceptable. Drop the now-unused `from functools import cached_property`. The coercion comment is retained. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

…te-info

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

thodson-usgs and others added 2 commits May 3, 2026 12:56

Simplify site_info lookup to a single loop

9695058

Folds the special-cased `siteid` branch into the loop over candidate keys; semantics are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

This was referenced May 4, 2026

Fix get_results: preserve user-supplied WQX3.0 dataProfile #250

Merged

Polish wqp.py: silence prints, fix exception shape, fix typos #256

Merged

thodson-usgs requested a review from Copilot May 4, 2026 14:56

Copilot started reviewing on behalf of thodson-usgs May 4, 2026 14:57 View session

Copilot AI reviewed May 4, 2026

View reviewed changes

thodson-usgs and others added 3 commits May 4, 2026 10:08

Merge remote-tracking branch 'origin/main' into fix/wqp-metadata-site…

add49eb

…-info # Conflicts: # tests/wqp_test.py

thodson-usgs requested a review from Copilot May 4, 2026 20:28

Copilot started reviewing on behalf of thodson-usgs May 4, 2026 20:29 View session

Copilot AI reviewed May 4, 2026

View reviewed changes

thodson-usgs and others added 2 commits May 4, 2026 20:31

Merge remote-tracking branch 'upstream/main' into fix/wqp-metadata-si…

3a26dde

…te-info

thodson-usgs requested a review from Copilot May 5, 2026 15:53

Copilot started reviewing on behalf of thodson-usgs May 5, 2026 15:54 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

thodson-usgs mentioned this pull request May 6, 2026

Release staging: PRs targeted for v1.1.4 #270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix WQP_Metadata.site_info: bind as a real property and forward kwargs#249

Fix WQP_Metadata.site_info: bind as a real property and forward kwargs#249
thodson-usgs wants to merge 7 commits intoDOI-USGS:mainfrom
thodson-usgs:fix/wqp-metadata-site-info

thodson-usgs commented May 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

thodson-usgs commented May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Minimal reproducible example

Pre-fix bug

Post-fix, live API

Test plan

Related PRs

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thodson-usgs commented May 3, 2026 •

edited

Loading