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

fix(installation)!: Return Result instead of panicking #687

Merged
merged 5 commits into from
Sep 30, 2024

Conversation

benpueschel
Copy link
Contributor

@benpueschel benpueschel commented Aug 30, 2024

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)

Refs: XAMPPRocky#641

BREAKING CHANGE: `Octocrab::installation` now returns a Result, changing
the public API.
@benpueschel
Copy link
Contributor Author

@XAMPPRocky Sorry for pinging you 😀
Is there any better way to create custom errors in octocrab?

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),
});

@XAMPPRocky
Copy link
Owner

@benpueschel We use the snafu crate currently for custom errors, so you can use that.

https://docs.rs/snafu/latest/snafu/

@benpueschel
Copy link
Contributor Author

I may be missing something here - does snafu provide a way to conveniently build the Error::Other variant?

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).

@XAMPPRocky
Copy link
Owner

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 #[non_exhaustive] to make that non breaking.

@benpueschel
Copy link
Contributor Author

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.
@benpueschel benpueschel marked this pull request as ready for review September 20, 2024 18:03
src/lib.rs Outdated Show resolved Hide resolved
@XAMPPRocky
Copy link
Owner

Thank you for your PR!

@XAMPPRocky XAMPPRocky merged commit 4164ea9 into XAMPPRocky:main Sep 30, 2024
11 checks passed
@github-actions github-actions bot mentioned this pull request Sep 30, 2024
@benpueschel benpueschel deleted the fix-installation branch October 6, 2024 16:00
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.

Return result instead of panic when returning an installation Octocrab instance
2 participants