enh: use the appropriate CURL options for specifying methods #357
Conversation
| @@ -99,7 +99,14 @@ print.request <- function(x, ...) { | |||
|
|
|||
| request_prepare <- function(req) { | |||
| req <- request_combine(request_default(), req) | |||
| req$options$customrequest <- req$method | |||
| switch(req$method, | |||
| "POST" = { | |||
There was a problem hiding this comment.
Could you please make it more like "POST" = (req$options$post <- TRUE) ?
There was a problem hiding this comment.
Why the brackets instead of curly? I'd rather have it be silent? I wouldn't mind putting it on one line if you prefer.
There was a problem hiding this comment.
I'm not sure what you mean by silent, but I don't think choice of parens matters.
There was a problem hiding this comment.
Curly brackets are expressions, they return the result invisibly. Just try:
switch("a", "a" = (tt <- 2))
# [1] 2
switch("a", "a" = {tt <- 2})
# Nothing
There was a problem hiding this comment.
Yes, but it really doesn't matter here. Regardless, splitting the expression over multiple lines really bugs me, so please fix.
|
Would you mind also adding a bullet point to NEWS? And any thoughts on how to test this? |
|
NEWS -> Will do |
|
Since testing is hard, could you add a comment mentioning why this behaviour is important? I'm worried I'll forget about it and re-simplify in the future. |
648dac5 to
7cd10ef
Compare
|
Thanks! |
|
Unfortunately this doesn't seem to be correct - setting I think this is probably because I'm setting the post body parameters in |
|
A bit of googling suggests that using |
|
Good catch - this is officially a Windows exception to the cURL behaviour, itself being a standard but non-compliant way of dealing with RFC... Why is software getting so crazy? |
cf #356.
A small fix can go a long way :-). I'd advocate for quick publishing to CRAN after inclusion of this PR (or whatever other code change that solves #356).