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

Introduce a way to distinguish different github errors #230

Merged
merged 3 commits into from
Dec 5, 2018

Conversation

mikegirkin
Copy link
Contributor

I discovered that there is not way to distinguish between different type of errors in the application using github4s.
This small PR introduces another type for the errors, so the consumer get more information to deal with:

  • ability to distinguish between HTTP errors (ie. file not found) and other
  • ability to deal with HTTP errors differently based on status code

Copy link
Contributor

@BenFradet BenFradet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that I think this just ends up being a rename of UnexpectedException if I'm not mistaken?

@mikegirkin
Copy link
Contributor Author

@BenFradet
Not exactly, I've added http code to be able to distinguish on the client between different situations with a valid response from github received.
UnexpectedException is used also in JS-specific part to recover from Future errors and save the message.

@BenFradet
Copy link
Contributor

BenFradet commented Nov 30, 2018

Ah yes forgot about those damned Futures, wdyt about renaming it to UnsuccessfulHttpRequest, Github being implied since it subclasses GHException?

@mikegirkin
Copy link
Contributor Author

@BenFradet Agree, I'll do that

@BenFradet
Copy link
Contributor

you need to rename the other 2 occurrences 👍

@mikegirkin
Copy link
Contributor Author

@BenFradet Silly mistake. Apologies.

Copy link
Contributor

@BenFradet BenFradet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran the tests successfully 👍

Copy link
Member

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mikegirkin !

@BenFradet BenFradet merged commit 2d62a2d into 47degrees:master Dec 5, 2018
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

Successfully merging this pull request may close these issues.

3 participants