Skip to content

Fix bugs, remove dead code, and deduplicate shared patterns#878

Merged
ldecicco-USGS merged 2 commits intoDOI-USGS:developfrom
thodson-usgs:simplify
Apr 16, 2026
Merged

Fix bugs, remove dead code, and deduplicate shared patterns#878
ldecicco-USGS merged 2 commits intoDOI-USGS:developfrom
thodson-usgs:simplify

Conversation

@thodson-usgs
Copy link
Copy Markdown

Summary

  • Fix format_api_dates bug where loop variable i was unused — full datetime vector passed to get_dateTime() instead of each element
  • Fix readNWISrating referencing undefined intColumns variable (would crash at runtime)
  • Remove dead functions (only_legacy, post_url, .capitalALL) and unreachable measurements switch branch
  • Extract add_api_token() helper replacing 4 duplicate token injection patterns
  • Extract coerce_num_cols() and log_rate_limit() helpers in walk_pages.R
  • Simplify deprecated read_USGS_samples wrapper from 60 lines to 5 using ... forwarding
  • Cache user-agent string in pkg.env to avoid recomputing on every HTTP request
  • Replace O(n²) rbind-in-loop with pre-allocated lists in readNGWMNdata

Net: -131 lines across 10 files. All 283 tests pass.

Test plan

  • All 283 existing unit tests pass (devtools::test())
  • Verify read_waterdata_samples deprecated alias still works
  • Verify datetime range queries produce correct ISO8601 output
  • Verify readNWISrating works when current_rating_nu column is present

🤖 Generated with Claude Code

@ldecicco-USGS
Copy link
Copy Markdown
Collaborator

Could you change the PR to aim to the "develop" branch? We try to keep "main" to what was most recently pushed to CRAN, and "develop" to what will eventually get pushed up there. The main reason for that (vs just a tagged release) is making it easier to keep the GH-Actions that produce the documentation sync'ed up to what's on CRAN and not a mis-match with what will be on CRAN (since the vast vast majority of users get the package from there and not GitHub).

I think the datetime bug was already fixed on develop. Some of the other shared pattern stuff looks good.

Fix format_api_dates bug where loop variable was unused and full
datetime vector was passed instead of each element. Fix readNWISrating
referencing undefined intColumns variable. Remove dead functions
(only_legacy, post_url, .capitalALL) and unreachable measurements
branch. Extract add_api_token, coerce_num_cols, and log_rate_limit
helpers to replace duplicated code. Simplify deprecated read_USGS_samples
wrapper to use ... forwarding. Cache user-agent string. Replace rbind
in loop with pre-allocated lists.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@thodson-usgs thodson-usgs changed the base branch from main to develop April 14, 2026 19:19
@thodson-usgs thodson-usgs marked this pull request as ready for review April 14, 2026 19:23
@thodson-usgs
Copy link
Copy Markdown
Author

thodson-usgs commented Apr 14, 2026

@ldecicco-USGS rebased to dev. Please leave comments on any changes you don't want, or close this PR entirely. I was mainly curious what Claude wouild drag up.

Give read_USGS_samples its own roxygen docs instead of sharing
@Rdname with read_waterdata_samples, since the ... signature
doesn't match the documented parameter list.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ldecicco-USGS ldecicco-USGS merged commit 32ad047 into DOI-USGS:develop Apr 16, 2026
1 check passed
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.

2 participants