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

Do not throw new Error() #400

Closed
samrocketman opened this issue Nov 20, 2017 · 5 comments
Closed

Do not throw new Error() #400

samrocketman opened this issue Nov 20, 2017 · 5 comments

Comments

@samrocketman
Copy link

samrocketman commented Nov 20, 2017 •
edited
Loading

https://github.com/kohsuke/github-api/search?utf8=%E2%9C%93&q=%22new+Error%22&type=

In general, this seems to be a bad idea. See also comments in jenkinsci/ghprb-plugin#570. This PR for the GHPRB plugin is left with the dilemma of catching throwable because there's no other choice since this API throws java.lang.Error.

A quote from java.lang.Error javadoc:

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch.

Instead, consider throwing IllegalStateException.
@jglick
Copy link
Contributor

jglick commented Nov 20, 2017

Care to file a PR for it?

@oleg-nenashev
Copy link
Collaborator

Well, even IllegalStateException is not ideal, checked exceptions would be better. But it requires introducing new APIs

@samrocketman
Copy link
Author

Yeah, I don't mind filing. I was going to bed last night so filed it before forgetting.

@bjoernhaeuser bjoernhaeuser mentioned this issue Nov 20, 2017
Closed
@samrocketman
Copy link
Author

Looks like PR has been filed.

@kohsuke
Copy link
Collaborator

kohsuke commented Jan 13, 2018

Merged #401

@kohsuke kohsuke closed this as completed Jan 13, 2018
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.

Replace "new Error" with GHException bjoernhaeuser/github-api
4 participants
@kohsuke @jglick @samrocketman @oleg-nenashev