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

app.Run() should return an error. os.Exit(1) should be avoided #39

Closed
codegangsta opened this issue Oct 29, 2013 · 3 comments · Fixed by #40
Closed

app.Run() should return an error. os.Exit(1) should be avoided #39

codegangsta opened this issue Oct 29, 2013 · 3 comments · Fixed by #40
Milestone

Comments

@codegangsta
Copy link
Contributor

@kytrinyx @msgehard This may affect you guys so I would like to know what you think about this.

I got bit recently by this, basically when an error occurs, like incorrect usage in a FlagSet or failing to find a help topic we call an os.Exit(1). This is convenient for the most part also terminates the app when you may not desire it.

The proposal is to basically replace the os.Exit(1) with a error return on app.Run(). That way these errors can be handled any way the developer wants to. This also feels a lot more idiomatic in terms of Go.

Let me know what you think. I would really love to get this to 1.0 So the api can get locked in.

@kytrinyx
Copy link
Contributor

I don't know enough to have a proper opinion, but I'm happy to adjust whatever I need to in the exercism CLI based on your choices. Returning an error rather than doing a hard exit does sound a lot more idiomatic.

@mikegehard
Copy link
Contributor

Returning an error from app.Run() sounds great to me as well. Thanks!

@codegangsta
Copy link
Contributor Author

Thanks for the input and thanks for using the package. I will start working on the changes soon. If you have any other feedback be sure to add an issue, I would love to lock down a 1.0 for cli so CF and Exercism won't neet to worry about breaking changes.

❤️

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 a pull request may close this issue.

3 participants