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.

Sign up for GitHub

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

Jump to bottom

Return result instead of panic when returning an installation Octocrab instance #641

Closed
krishmas opened this issue May 16, 2024 · 1 comment · Fixed by #687
Closed

Return result instead of panic when returning an installation Octocrab instance #641

krishmas opened this issue May 16, 2024 · 1 comment · Fixed by #687

Comments

@krishmas
Copy link

octocrab/src/lib.rs

Line 981 in 7e280be

panic!("Github App authorization is required to target an installation");

Would be nicer to return an Error result (like other functions that return different types of Octocrabs do) instead of panicking so the application using doesn't immediately crash out
@XAMPPRocky
Copy link
Owner

Thank you for your issue! A PR adding this would be welcome 🙂

benpueschel added a commit to benpueschel/octocrab that referenced this issue Aug 30, 2024
@benpueschel
fix(installation)!: Return Result instead of panicking
0e9243d 
Refs: XAMPPRocky#641

BREAKING CHANGE: `Octocrab::installation` now returns a Result, changing
the public API.
@benpueschel benpueschel mentioned this issue Aug 30, 2024
Merged
XAMPPRocky pushed a commit that referenced this issue Sep 30, 2024
@benpueschel
fix(installation)!: Return Result instead of panicking in `Octocrab::…
4164ea9 
…installation` (#687)

* fix(installation)!: Return Result instead of panicking

Refs: #641

BREAKING CHANGE: `Octocrab::installation` now returns a Result, changing
the public API.

* refactor(error)!: add `installation` variant

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.

* style: remove unused import

* style: remove unnecessary error let binding

* refactor: remove installation error message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(installation)!: Return Result instead of panicking benpueschel/octocrab
2 participants
@XAMPPRocky @krishmas