Fix _arrange_cols: stop mutating the caller's properties list#254
Draft
thodson-usgs wants to merge 4 commits intoDOI-USGS:mainfrom
Draft
Fix _arrange_cols: stop mutating the caller's properties list#254thodson-usgs wants to merge 4 commits intoDOI-USGS:mainfrom
thodson-usgs wants to merge 4 commits intoDOI-USGS:mainfrom
Conversation
`_arrange_cols` did `properties.append("geometry")` and
`properties[properties.index("id")] = output_id` in place. Calling any
getter twice with the same `properties=[...]` therefore caused that
list to grow unboundedly and have `id` rewritten across calls — a
real action-at-a-distance defect for any code that re-uses a
`properties` list across requests.
Take a local copy of the list and mutate that. Caller's list is
untouched.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the regression-narrative phrasing for the local-copy comment and trim the multi-line "id is technically a valid column..." block to a two-line WHY explaining the rename intent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 4, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an action-at-a-distance bug in dataretrieval.waterdata.utils._arrange_cols where the function mutated the caller-provided properties list in-place, causing surprising behavior when the same list instance is reused across multiple requests.
Changes:
- Stop mutating the caller’s
propertieslist by copying it to a local list before applyingid/geometryadjustments. - Add regression/behavior tests covering non-mutation,
id→output_idbehavior, and retention ofgeometrywhen present.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
dataretrieval/waterdata/utils.py |
Copies properties before mutation to prevent side effects across calls. |
tests/waterdata_utils_test.py |
Adds regression + behavior tests validating the fixed _arrange_cols semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -687,15 +687,15 @@ def _arrange_cols( | |||
| # If properties are provided, filter to only those columns | |||
| # plus geometry if skip_geometry is False | |||
Per copilot review on PR DOI-USGS#254. _arrange_cols doesn't take skip_geometry - it conditionally keeps the geometry column based on whether one is present in the DataFrame. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tion # Conflicts: # tests/waterdata_utils_test.py
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
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.
Summary
`_arrange_cols` did `properties.append("geometry")` and `properties[properties.index("id")] = output_id` in place. Calling any getter twice with the same `properties=[...]` therefore caused that list to grow unboundedly and have `id` rewritten across calls — a real action-at-a-distance defect for any user code that re-uses a `properties` list across requests.
This PR takes a local copy of the list and mutates that. The caller's list is untouched.
Test plan
Related PRs
Other open PRs in this bug-review series that touch
dataretrieval/waterdata/utils.py(different functions, no functional conflicts):_format_api_datesaccept ISO 8601._arrange_colsstop mutating caller list._handle_stats_nestingtolerate missing drop columns.