Cobra: Command.Run should return error

[bep]

The error handling in general confuses me.

• There is no obvious way to propagate errors from Command.Run upwards, consider changing signature to return error
• If Command.Run fails with an error, exit with exit code != 0

[edgard]

:+1: this is very welcomed. I cringe everytime I have to do error processing on my inner functions. :D

[bep]

Yes, well - I don't see a way to change this without breaking a lot of user code ...

To my surprise, the "other Go CLI framework", seems to be doing exactly the same.

[bep]

A little surprised to see so little interest in this issue.

@eparis what do you to handle this problem? Handle all errors inside the commands?

[eparis]

For the most part, everything I do called

if err := cmd.Execute(); if err != nil {
   os.Exit(1)
}
os.Exit(0)

And inside run we've got glog.Fatal() (which does os.Exit(1) on unhandl-able errors)

[j16r]

Perhaps an alternative Run command, e.g:

RunWithError: func(cmd *cobra.Command, args []string) error {

(not the best name to be sure)

[apriendeau]

I would agree with this but it would be a breaking news change. We should always populate the errors up. @eparis a good use case would be testing but also populating the errors up would allow for a single error handling for parent and child commands as well.

[pgruenbacher]

Agreed, I'm using a fork for the time being. I prefer to have a single os.Exit on a single error exit point.
https://github.com/pgruenbacher/cobra

[eparis]

So I don't think we can/should change Run. Too many people are using it I'd expect. Maybe we should add a RunE or something like that? Same thing as run but which returns an error which gets populated back up through Execute() ?

[eparis]

https://gist.github.com/eparis/be6ca4c767e69fcaaea8 ?? Maybe something like that?

[bep]

@eparis agree. RunE also matches the naming convention I've seen elsewhere in situation like this.

[apriendeau]

@eparis @bep I agree. That works if we want to use that but I think it would be better in the longer term to make that the default. Possibly a deprecation period?

[apriendeau]

I think that the only thing is that patch would need to have is these: PostRunE, PersistentPostRunE, PreRunE and PersistentPreRunE to achieved the full desired effect.

[apriendeau]

@bep @eparis I think this should be good to close with the fix from Alex, right?