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

HTTPError should be unwrappable. #97

Merged
merged 2 commits into from
Apr 7, 2023
Merged

Conversation

cube2222
Copy link
Contributor

@cube2222 cube2222 commented Apr 6, 2023

Hey @bradleyfalzon!

Thanks for creating https://github.com/bradleyfalzon/ghinstallation!

An issue we've been having is that, when using a custom http client underneath ghinstallation and returning special error types from it, ghinstallation wraps them in HTTPError which we can't unwrap using errors.As.

Would you consider accepting this fix upstream?

Signed-off-by: Jakub Martin <kubam@spacelift.io>
@bradleyfalzon
Copy link
Owner

Yeah I think this is entirely reasonable.

Any chance on a small comment on Unwrap, like “Unwrap returns RootCause the underlying error”.

I also can’t see any tests for HTTPError, this looks like we never added one initially. Any chance you could spend a moment and add an appropriate test for the specific Unwrap method?

@wlynch any thoughts?

Signed-off-by: Jakub Martin <kubam@spacelift.io>
@cube2222
Copy link
Contributor Author

cube2222 commented Apr 7, 2023

@bradleyfalzon Sure, here you go.

@bradleyfalzon
Copy link
Owner

Thanks @cube2222, I’ll see if @wlynch has any comments if he has a moment, else I’ll merge in and tag in next few days, unless you’re in a rush.

@cube2222
Copy link
Contributor Author

cube2222 commented Apr 7, 2023

Awesome, thanks @bradleyfalzon! No rush, using our fork in the meantime.

@wlynch
Copy link
Collaborator

wlynch commented Apr 7, 2023

LGTM! Thanks for adding this!

@wlynch wlynch merged commit 535c380 into bradleyfalzon:master Apr 7, 2023
1 check passed
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.

None yet

3 participants