Troubleshoot new deck API endpoint #630
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #630 +/- ##
==========================================
- Coverage 90.96% 90.91% -0.05%
==========================================
Files 128 128
Lines 8490 8502 +12
==========================================
+ Hits 7723 7730 +7
- Misses 767 772 +5
☔ View full report in Codecov by Sentry. |
| @@ -50,17 +50,31 @@ crunchAPI <- function( | |||
| crGET <- function(...) crunchAPI("GET", ...) | |||
| #' @rdname http-methods | |||
| #' @export | |||
| crPUT <- function(...) crunchAPI("PUT", ...) | |||
| crPUT <- function(url, config = list(), ..., body) { | |||
There was a problem hiding this comment.
arg order is weird here, but trying to match crunchAPI() so that I don't make any breaking changes
There was a problem hiding this comment.
Does body need to be named as opposed to a part of ...?
There was a problem hiding this comment.
This is the easiest way I know how to avoid evaluating the arguments inside ... at the wrong time, I want to just pass them onto the next function as is, but eg list(...) evaluates all of them. (The only other way I know is to use ...names() & ...n() to only evaluate body, but it looks uglier)
| @@ -39,7 +39,7 @@ crunchAPI <- function( | |||
| #' These methods let you communicate with the Crunch API, for more background | |||
| #' see [Crunch Internals](https://crunch.io/r/crunch/articles/crunch-internals.html). | |||
| #' | |||
| #' @param ... see [`crunchAPI`] for details. `url` is the first | |||
| #' @param url,config,body,... see [`crunchAPI`] for details. `url` is the first | |||
There was a problem hiding this comment.
The help page now looks a bit odd (compare GET/DELETE & PUT/PATCH/POST, they all get url/config, but it's explicit only in the latter group).
There was a problem hiding this comment.
Yeah, do you think it is worth making url/config explicit in all?
There was a problem hiding this comment.
Made them consistent at least
| if (code == 404) { | ||
| # Add the URL that was "not found" (there isn't going to be any | ||
| # useful response content message) | ||
| msg2 <- response$url | ||
| } else { | ||
| msg2 <- try(content(response)$message, silent = TRUE) | ||
| err_content <- try(content(response), silent = TRUE) |
There was a problem hiding this comment.
Are both of the checks needed in the top-level if statement? I'd think is.list is enough ...
Do we lose anything with a more radical simplification such as:
if (is.list(err_content)){
msg2 <- coalesce(err_content$message, err_content$description)
}
... adding coalesce somewhere:
coalesce <- function(...){
for (i in list(...)) if (!is.null(i)) return(i)
}
There was a problem hiding this comment.
Cool, I'll take another look. Partly I'm trying to be as defensive as possible because an error here would mean an unintelligible error to the user
There was a problem hiding this comment.
Adapted some, but really want to be conservative here because we're by definition working with a response we don't fully understand (because we got an error) and it makes super confusing behavior for the user if we error in our intrepration
| } | ||
| if (!is.error(msg2)) { | ||
|
|
||
| if (!is.null(msg2)) { | ||
| msg <- paste(msg, msg2, sep = ": ") |
There was a problem hiding this comment.
Random rant: I always found it annoying that paste includes the separator even with NULL. If paste('foo', NULL, sep = ':') returned 'foo', you wouldn't need an if statement in cases like this one.
gergness
force-pushed
the
rcrunch-scriptable-deck-185973710
branch
from
dadcc82
to
ed2b0e2
Compare
No description provided.