-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
fix(installation)!: Return Result instead of panicking #687
Conversation
Refs: XAMPPRocky#641 BREAKING CHANGE: `Octocrab::installation` now returns a Result, changing the public API.
@XAMPPRocky Sorry for pinging you 😀 I'm not really happy with the way I did it as a first quick draft: let source = std::io::Error::new(
std::io::ErrorKind::Other,
"Github App authorization is required to target an installation",
);
return Err(Error::Other {
backtrace: Backtrace::generate_with_source(&source),
source: Box::new(source),
}); |
@benpueschel We use the snafu crate currently for custom errors, so you can use that. |
I may be missing something here - does snafu provide a way to conveniently build the If it doesn't, I could maybe add a new Error type (though I don't really like that - it's both not elegant and would introduce another breaking change for anyone matching on the octocrab Error type). |
I would add an error variant. We should change the error enum to be |
Sounds good. |
Adds a new variant `Installation` on the Error enum and also makes the enum non-exhaustive to prevent further breaking changes when adding new variants in the future.
Thank you for your PR! |
As #641 describes, returning a Result when calling
Octocrab::installation
is nicer than panicking outright.I'm definitely not happy with how I build and return the Errors - is there a better way to build Errors directly?
This PR changes the public API behavior, since
Octocrab::installation
is exposed to the user.(closes: #641)