octocrab icon indicating copy to clipboard operation
octocrab copied to clipboard

fix(installation)!: Return Result instead of panicking

Open benpueschel opened this issue 1 year ago • 3 comments

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.

benpueschel avatar Aug 30 '24 13:08 benpueschel

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

benpueschel avatar Aug 31 '24 15:08 benpueschel

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

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

XAMPPRocky avatar Sep 01 '24 09:09 XAMPPRocky

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

benpueschel avatar Sep 03 '24 14:09 benpueschel

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.

XAMPPRocky avatar Sep 20 '24 09:09 XAMPPRocky

Sounds good.

benpueschel avatar Sep 20 '24 15:09 benpueschel

Thank you for your PR!

XAMPPRocky avatar Sep 30 '24 14:09 XAMPPRocky