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

Response::error is a footgun #14

Closed
jyn514 opened this issue Aug 24, 2021 · 2 comments
Closed

Response::error is a footgun #14

jyn514 opened this issue Aug 24, 2021 · 2 comments

Comments

@jyn514
Copy link
Contributor

jyn514 commented Aug 24, 2021

Response::error("Error", 200) is perfectly valid and is not correct at all.

Actually, looking at the implementation it would be fine if we just renamed this to Response::with_status. @nilslice what do you think? We could still have Response::internal_error or Response::request_error convenience wrappers if you think they're helpful.

(This may become a moot point after implementing #13.)

@nilslice
Copy link
Contributor

Yea I think renaming this to Response::with_status is fine. I like having an error method though for spot-checking code, making it really easy to know immediately that a certain branch is returning a failure indicator.

Feel free to rename, and if you want to, we could add a Response::error method which verifies that the code is within a true-to-spec error range. 400 - 599 would be a good start.

@nilslice
Copy link
Contributor

For now I've updated Response::error so it is only valid for error-related status codes. Adding a with_status method is a good idea too and I will probably include it along with some other clean-up work.

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

2 participants