Skip to content
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

more-flexible framework for error-checking #277

Closed
ijlyttle opened this issue Sep 8, 2015 · 8 comments
Closed

more-flexible framework for error-checking #277

ijlyttle opened this issue Sep 8, 2015 · 8 comments

Comments

@ijlyttle
Copy link

ijlyttle commented Sep 8, 2015

It would nice to be able to customize error messages, so I made an addon package: https://github.com/ijlyttle/httrplus to do this.

My goal was to make sure that this approach would work (it seems to), and then to propose it here.

If it is thought to be reasonable way to go, I can make a PR to this repo (of course, incorporating feedback as needed).

It may be useful to have some boilerplate message-functions - I include a couple proposals:

  • message_standard() - just a rewrite of what is already in httr
  • message_verbose() - a first pass at a more-verbose message.

Excerpt from README at https://github.com/ijlyttle/httrplus

The purpose is to propose a framework around httr::stop_for_status() and httr::warn_for_status(). The aim is for you to be able to more-easily customise messsages and error/warning-signalling behaviour when using httr.

The central function in this package is check_for_status(). Its arguments are:

  • resp - a httr::response object
  • type - one of c("error", "warning", "message")
  • message_function - a function that takes resp as its first argument, returns message
  • ... - additional arguments to message_function

The default message_function is message_standard() (provided here), a function that replicates the messages returned by httr::stop_for_status() and httr::warn_for_status()

Hence, the existing httr functions could be rewritten:

stop_for_status <- function(x){
  check_for_status(x, type = "error")
}

warn_for_status <- function(x){
  check_for_status(x, type = "warning")
}
@hadley
Copy link
Member

hadley commented Sep 9, 2015

Could you explain a bit what problem this solves?

@ijlyttle
Copy link
Author

ijlyttle commented Sep 9, 2015

The standard message (stop_for_status, warn_for_status) tells you the error code and a standard description associated with the code. There can be more information available in the API response to help pinpoint the problem (maybe the user has exceeded the quota for the day), but the location and format of this information may vary from API to API, and call to call.

This way, the httr user need only supply a formatting function for the message (which may be particular to that API-call), but will not have to recreate the entire error-checking framework.

Another way to do it may be to allow a custom message-function as an additional arg to stop_for_status and warn_for_status.

@hadley
Copy link
Member

hadley commented Sep 9, 2015

I still don't get it. If you're writing an API wrapper, you're going to have to wrap many other things too.

@jeroen
Copy link
Member

jeroen commented Sep 9, 2015

Annotating an assertion with a little description to enhance the error message is not uncommon. For example if you are writing a client for you kitchen utilities API:

req <- GET("http://httpbin.org/status/418")
stop_for_status(req, "espresso machine authentication")

That would result in:

Error: espresso machine authentication error: (http 418) I'm a teapot (RFC 2324)

This can make it just a little bit easier for the user to understand where things did go wrong. Not sure if this is what OP has in mind, and if httr is the best place for this.

@ijlyttle
Copy link
Author

ijlyttle commented Sep 9, 2015

This is sort-of what I had in mind, but with the thought that the second arg to stop_for_status could be a function.

To bounce off of the espresso example:

# this is the user's responsibility to provide
message_espresso <- function(x) {

  # here, the user can dig all sorts of stuff out of the response - this is the standard message
  status <- httr::status_code(x)
  msg <- httr::http_status(status)$message

  paste0("espresso machine: ", msg)
}

req <- GET("http://httpbin.org/status/418")
stop_for_status(req, message_espresso)

The thought is for the user to be able to rely on the existing httr framework to throw the error/warning - and to be responsible only for providing a function to compose the message.

To echo @jeroenooms, I don't know either if httr is the best place for this, but I do appreciate the discussion.

@jennybc
Copy link
Member

jennybc commented Oct 3, 2015

FWIW I think this would be quite useful. I would use it right now if were there. I wish I could do something like this:

stop_for_status(req, 403,
                "Check that your Sheet is 'published to the web', not just 'public on the web'.")

i.e. conveniently issue a customized message for a specific status code in a specific situation. Yes we can just do this while we are wrapping everything else but this would be quite nice.

@hadley
Copy link
Member

hadley commented Dec 17, 2015

Have you seen the example in http_condition()? I think that's the right way to handle things. Otherwise, if that doesn't work for you, could you please provide a few more concrete examples and maybe a sketch of an implementation for review?

@jeroen
Copy link
Member

jeroen commented Dec 18, 2015

Discussion moved to #302.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants