-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial switch to httr2 #738
Merged
Merged
Changes from 28 commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
030edf4
First steps in switching to httr2
ldecicco-USGS 84cb3cc
Getting there!
ldecicco-USGS ffbfaa3
udpates from main
ldecicco-USGS a8f236e
update ua
ldecicco-USGS ea8170a
start
ldecicco-USGS d829f26
Merge branch 'main' of https://code.usgs.gov/water/dataRetrieval into…
ldecicco-USGS 6062607
cleaning up WQP calls
ldecicco-USGS af0b523
style
ldecicco-USGS f1bbab0
getting the lists correct
ldecicco-USGS 993a2a4
More NWIS and WQP httr2 updates
ldecicco-USGS 9fca4e2
a few httr2 updates
ldecicco-USGS 313fdda
Fixing some tests. Mostly arguments in the URLs are shuffled around.
ldecicco-USGS f09bb35
wrong argument
ldecicco-USGS 90cb26c
More tests to fix and update findNLDI
ldecicco-USGS 746d32c
local file tests
ldecicco-USGS 47e35a7
getting water use URLs to work
ldecicco-USGS 790f924
more test url updates
ldecicco-USGS 82318a9
everything except NGWMN working
ldecicco-USGS 9792060
NGWMN
ldecicco-USGS 111feb5
Dropping problimatic example (data source keeps changing)
ldecicco-USGS 6bfc32a
Upstream pull
ldecicco-USGS d289745
adding count=no to legacy
ldecicco-USGS 7ad1ed5
updating docker file
ldecicco-USGS 20fa9e7
run check on develop
ldecicco-USGS 8f93afe
Fix test due to my last minute addition of count=no to legacy WQP
ldecicco-USGS 8ee4a7a
change example
ldecicco-USGS 0b60de4
Another set of multi's
ldecicco-USGS 6146ebb
wqp_check_status example
ldecicco-USGS 722ed47
Trying new GH actions to reduce suggest list, and responding to a few…
ldecicco-USGS 0b30821
Shame, shame, shame! 😔
ldecicco-USGS 1704e18
Meant to include this so we can test if the GH actions work (building…
ldecicco-USGS 29ecfcb
More updates thanks to Joe's review
ldecicco-USGS 9ce4776
Custom .multi for legacy
ldecicco-USGS File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth, httr2 (and the tidyverse) now require R >= 4.0, and in the spring will require R >= 4.1 . If you bump this to 4.1 (which, depending when you're planning on launching things, will be the minimum version to use the package anyway) then you can use the base pipe, which might make some of the query-building more fluid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳I've been avoiding the 4.1 requirement, but if httr2 has it already....HELLO PIPES
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also re-export the magrittr pipe from httr2 (either directly or by adding magrittr as a direct dependency) if this is going out before the spring. Here's how httr2 does it:
https://github.com/r-lib/httr2/blob/main/R/utils-pipe.R
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When magrittr first came out, we very firmly came to the conclusion that we would never use it within a package, so I've very much avoided re-exporting it. However, my understanding is that the base R pipe does a better job of allowing the "traceback" when an error occurs. So, I'm happy to start using native pipes - but would not introduce magittr at this stage of the game.