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

Annotated assertions in stop_for_status #302

Closed
jeroen opened this Issue Dec 18, 2015 · 4 comments

Comments

Projects
None yet
3 participants
@jeroen
Member

jeroen commented Dec 18, 2015

Copy from comment.

It makes more readable code if the annotation in stop_for_status is not an error message but an assertion statement. When writing code we don't know the error, we only know what it was that we were trying to do in the request. For example:

stop_for_status(req, "download spreadsheet")
## Error: download spreadsheet failure: not found (HTTP 404)

or:

stop_for_status(req, "download spreadsheet")
## Error: download spreadsheet failure: permission denied (HTTP 401)

or:

stop_for_status(req, "download spreadsheet")
## Error: download spreadsheet failure: service unavailable (HTTP 503)

Sorry for not being more clear about this. I think the current implementation stop_for_status(x, "Spreadsheet not found") is often impractical because typically we can't know the exact error cause in advance. If we do have a mapping between status code and error messages, we are better of using http_condition.

The goal of stop_for_status messages would be to encourage authors of client libraries to let the user know what/where an error was created. Generic http errors messages are not too helpful for high level wrappers:

generate_report(foo, bar, "my_data")
## Error: Permission Denied (HTTP 401)

Instead, the user would see a more informative message that states what exactly what went wrong:

generate_report(foo, bar, "my_data")
## Error: download spreadsheet failure: permission denied (HTTP 401)
@hadley

This comment has been minimized.

Member

hadley commented Dec 18, 2015

I think the ideal solution would be something like:

generate_report(foo, bar, "my_data")
## HTTP Failure: download spreadsheet failure (401: permission denied)

But unfortunately "Error: " is hard-coded in C.

I don't think "assertion statement" is quite the right term, but I get where you're coming from. I think you want to state what the goal is, and then httr should incorporate that into the message:

stop_for_status(req)
# Error: Request failed (HTTP 404: file not found)
stop_for_status(req, goal = "Downloading spreadsheet")
# Error: Downloading spreadsheet failed (HTTP 404: file not found)

It's just a matter of getting the English phrasing right so it doesn't feel too awkward. I feel like just tacking "failed" on to the end is suboptimal - i.e. you want goal = "download spreadsheet", so maybe

  • Failed to download spreadsheet
  • Failed to issue request

But the default isn't quite right - the request was issued successfully, just the response wasn't as you'd hoped.

@ijlyttle

This comment has been minimized.

ijlyttle commented Dec 20, 2015

I like the goal argument.

Just to throw out other phrasing ideas:

  • Unable to ...
  • Did not ...

If you wanted to be more complicated, you could distinguish between the 4xx and 5xx codes -

  • Client unable to ... / Server unable to ...

maybe a default for goal could be "complete requested task"

stop_for_status(req)
# Error: Client failed to complete requested task (HTTP 404: file not found)
stop_for_status(req, goal = "download spreadsheet")
# Error: Client failed to download spreadsheet (HTTP 404: file not found)
@hadley

This comment has been minimized.

Member

hadley commented Jan 8, 2016

How about this:

stop_for_status(req)
# Error: File not found (HTTP 404).
stop_for_status(req, task = "download spreadsheet")
# Error: File not found (HTTP 404). Failed to download spreadsheet.
@jeroen

This comment has been minimized.

Member

jeroen commented Jan 8, 2016

Looks good

@hadley hadley closed this in d5bf17b Jan 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment