Improve error messages #30
Conversation
R/InstanceAPI.R
Outdated
| if (httr::http_error(request)) { | ||
| cli_abort(content$detail) | ||
| } | ||
| request <- httr::GET(url) |
There was a problem hiding this comment.
Should this be "response" rather than "request"?
R/InstanceAPI.R
Outdated
| process_request = function(request, request_type) { | ||
| content <- httr::content(request) | ||
| if (httr::http_error(request)) { | ||
| cli_abort(content$detail) | ||
| if (is.list(content) && "detail" %in% names(content)) { | ||
| cli_abort(content$detail) | ||
| } else { | ||
| cli_abort(paste0("Failed to ", request_type, " from instance. Response output: ", content)) | ||
| } |
There was a problem hiding this comment.
I'm not sure how this will work with the API package but we can sort that out later
R/InstanceSettings.R
Outdated
| cli_abort("Missing column: ", missing_column) | ||
| missing_key <- setdiff(expected_keys, names(settings)) | ||
| if (length(missing_key) > 0) { | ||
| cli_abort(paste0("Missing key: ", paste(missing_key, collapse = ", "))) |
There was a problem hiding this comment.
Missing keys? I think {cli} can automatically pluralise stuff if you want to get fancy but not sure it's worth the effort.
There was a problem hiding this comment.
Oh, this is cool, I didn't know {cli} could do this!
tests/testthat/test-instance_api.R
Outdated
| ) | ||
| } | ||
|
|
||
| test_that("test get_schema", { |
There was a problem hiding this comment.
Definitely not important but I think you are supposed to read this as a sentence starting with "Test that..." so the extra "test" is a bit redundant.
R/InstanceSettings.R
Outdated
| if (length(missing_column) > 0) { | ||
| cli_abort("Missing column: ", missing_column) | ||
| missing_keys <- setdiff(expected_keys, names(settings)) | ||
| if (length(missing_key) > 0) { |
There was a problem hiding this comment.
Missing a s here
| if (length(missing_key) > 0) { | |
| if (length(missing_keys) > 0) { |
There was a problem hiding this comment.
Would accept this suggestion but already fixed it myself based on the feedback you gave
* origin/main: Add printing to remaining classes (#31) Improve error messages (#30) Update documentation (#29) Return `NULL` when a record's related field is empty (#28) minor fix in usage vignette (#32) Add usage vignette (#18) Add a simple unit test which queries laminlabs/lamindata (#27) bump action from v4.5.0 to v4 (#26)
Changes
Added unit test for the InstanceAPI class (PR Improve error messages #30).
Add alternative error message when no message is returned from the API (PR Improve error messages #30).