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 shouldn't exit if error doesn't implement ExitCoder #594

Closed
awsmsrc opened this issue Jan 31, 2017 · 4 comments
Closed

App shouldn't exit if error doesn't implement ExitCoder #594

awsmsrc opened this issue Jan 31, 2017 · 4 comments

Comments

@awsmsrc
Copy link

awsmsrc commented Jan 31, 2017

Current behavior:

Even if the error returned doesn't implement ExitCoder the app exits with an exit status of 1

Expected behavior

the app will return the error but not exit so it can be introspected as follows

err := app.Run()
if err != nil {
  if err.IsInterestingToMe() {
    reportInterestingData()
  } else {
    exit(1)
  }
}

Usecase

if the users token has expired, prompt for a email/password, generate a new token and re run the command

Alternative option

Allow for a custom HandleExitCoder to be specified

@awsmsrc
Copy link
Author

awsmsrc commented Feb 2, 2017

Happy to help with whatever implementation is decided here, if it can help get things moving :)

jszwedko added a commit that referenced this issue Feb 4, 2017
Doing so limits the ability for users to have only some of their errors
cause the application to terminate while allowing others to bubble up.

This was originally an adjustment to #361 in #496 to fix #495, but, in
hindsight, I believe that the better approach is to recommend the use of
`RunAndExitOnError` for this use case (which is undeprecated here).

Fixes #565 #594
@jszwedko
Copy link
Contributor

jszwedko commented Feb 4, 2017

@awsmsrc 👍 thanks for filing this -- it reminded me to take a deeper dive into #565 and I came to the same conclusion you did. #595 adjusts this behavior to only exit for ExitCoders.

@kklipsch
Copy link
Contributor

kklipsch commented Mar 16, 2017

So #595 has the effect of ignoring errors coming out of the action function unless they meet the ExitCoder interface? That seems weird and wrong. Why not change the action function to explicitly require an ExitCoder?

For the record, both behaviors introduce regressions. If someone is programming against the current release changing this behavior breaks their code.

@jszwedko
Copy link
Contributor

jszwedko commented Apr 24, 2017

@kklipsch it doesn't ignore the errors, rather it propagates them up to the App.Run() method so that the caller can decide what to do. You can use App.RunAndExitOnError as a convenience to exit with a code of 1 if desired.

I agree that #595 breaks the interface, but #565 broke it initially and I think the least harmful approach is to put the behavior back to not surprise people who are upgrading (as well as not removing the previously supported feature of deciding what to do with errors).

Closing this since #595 has been merged in.

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

No branches or pull requests

3 participants