-
Notifications
You must be signed in to change notification settings - Fork 0
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
005 switch to using httr2 #6
base: main
Are you sure you want to change the base?
Conversation
Merge branch 'main' into 005_switch_to_using_httr2 # Conflicts: # R/request.R
@danymukesha: see the req.R file. I think we should go with functions along the lines of There is an example of how you would run The I don't think the Ensembl REST API has paginated responses, so that's one less headache. But it has throttling: https://github.com/Ensembl/ensembl-rest/wiki/Rate-Limits. So we should try to see how to accommodate that. But let me know what do you think about these two functions first: |
@ramiromagno I believe we can reuse it for both GET and POST requests. We might also consider adding assertions and some error handling to improve user-friendliness. I am not totaly sure about the Ensembl REST API not having paginated responses. Because, I think that if the response would contain huge genomic sequence(s) with formats like fasta (e.g: https://rest.ensembl.org/documentation/info/sequence_region), then (I guess) we should expect some kind of pagination. |
Hi @danymukesha! Really great work! I did quite a few changes but most are stylistic. The code and tests were very good already. Here are a few points for discussion:
If you agree, I think next steps:
The general idea is the following:
The scope of this PR is getting the following functions implemented, documented and tested:
|
@ramiromagno take an example we have
I then expect to have the following combinations of multiple requests:
Meaning we expect to have 4 different requests, right? But in our case we have only two combinations: > reqs(res, var1 = var1, var2 = var2)
[[1]]
<httr2_request>
GET https://rest.ensembl.org/endpoint/a/x
Headers:
• Content-Type: 'application/json'
Body: empty
Options:
• useragent: 'ensemblr (https://www.pattern.institute/ensemblr)'
[[2]]
<httr2_request>
GET https://rest.ensembl.org/endpoint/b/y
Headers:
• Content-Type: 'application/json'
Body: empty
Options:
• useragent: 'ensemblr (https://www.pattern.institute/ensemblr)' Is that making sense? |
Yeah, unless we have strong arguments for that, I would stick with https://vctrs.r-lib.org/reference/theory-faq-recycling.html. |
If get() would take a body argument, then it would become a post(), isn't it? |
Yeah I could confirm that. Providing the body argument defines the method to use automatically. https://github.com/r-lib/httr2/blob/f6fb40865ef59eda55f10e6f846361a5718c078d/R/req-body.R#L11 |
Do you think we need to assert the input arguments on |
I switched from
httr
tohttr2
for the function request. There is a remaining part for the error handling that I should complete.json
content from the response instead of text.