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

Strict mode - Throw error if response isn't 2xx #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

XuluWarrior
Copy link

No description provided.

@ethanent
Copy link
Owner

ethanent commented Jul 17, 2021

EDIT: I looked more closely at how you handled the strict option. This looks like a reasonable solution to me. It should satisfy the complaints about attempting to parse JSON for non-200 responses while still providing expected behavior for those who don't have the same requirement.

@jdforsythe
Copy link
Contributor

jdforsythe commented Jul 19, 2021

@ethanent @XuluWarrior I agree this would be a good solution. The only comment I'd have is that the user may want to be able to inspect the response, which would get lost with this solution. Adding a class that extends Error and attaches the response to it would make the response available if the user wants to inspect it.

It might even be helpful to have separate ClientError for 4xx and ServerError for 5xx. I'm not sure if you'd want to lump 1xx and 3xx into a single "other" error or separate InformationalResponseError and RedirectResponseError. This would allow instanceof checks for easier error handling - for instance the user may want to retry on a ServerError but a ClientError indicates retrying would be futile.

@jumoog
Copy link

jumoog commented Jul 23, 2021

I like the idea. And agree with @jdforsythe on separate ClientError for 4xx and ServerError for 5xx.

@XuluWarrior
Copy link
Author

I kind of put this PR down after my initial attempt worked well enough for me but I think it is still worth finishing.

I agree that extending Error with a NetworkError/ConnectionError? type would be useful. Although, I'm not so sure about the benefit of having separate ClientError and ServerError.

There are client errors where simply retrying later has a chance of success (e.g. 425 "To Early" and 429 "To Many Requests"). And there are server errors which require client action (e.g. 505 "HTTP Version Not Supported" and 511 "Network Authentication Required")
As a developer, I think I would be more interested in the exact code than the category of response.

@XuluWarrior
Copy link
Author

XuluWarrior commented Jan 2, 2024

I'd forgotten that I'd created this PR but I still believe that "strict mode" really helps streamline code that uses phin. (I'm happily using my fork for this reason)

Any thoughts as whether this can be merged in some form? I'm happy to put the additional effort to address review comments.

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

Successfully merging this pull request may close these issues.

4 participants