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

Silence some usage and error logging in Execute #168

Closed
wants to merge 1 commit into from
Closed

Silence some usage and error logging in Execute #168

wants to merge 1 commit into from

Conversation

bep
Copy link
Collaborator

@bep bep commented Oct 17, 2015

RunE was introduced on Command to support centralized error handling.

The errors returned from RunE is, however, still logged by Cobra, and there is no easy way to toggle the usage logging on a case-by-case basis (other than the brutal os.Exit(-1).

This commit introduces two small interfaces that enables end users to signal the behavior they want from Cobra in this area.

`RunE` was introduced on `Command` to support centralized error handling.

The errors returned from `RunE` is, however, still logged by Cobra, and there is no easy way to toogle the usage logging on a case-by-case basis (other than the brutal `os.Exit(-1)`.

This commit introduces two small interfaces that enables end users to signal the behavior they want from Cobra in this area.
@CLAassistant
Copy link

CLA assistant check
All committers have accepted the CLA.

@bep
Copy link
Collaborator Author

bep commented Oct 17, 2015

@eparis I have no idea what got that circleci test to fail.

About the PR: This is my suggestion. I have started the job of getting rid of all the os.Exit(-1) in Hugo, and I need better control of the logging in Cobra.

I know I could add some configuration, and that would maybe work for the error logging case, but the need for usage logging is on a "case-by-case" basis ... So this is my best current solution. Open to alternative suggestions.

@apriendeau
Copy link
Contributor

Personally, I would rather see a field in the command struct vs creating interfaces. I think it should be on each command and subcommands should inherit from the root cmd. For consistency.

@eparis
Copy link
Collaborator

eparis commented Oct 26, 2015

I like the SilenceError (and maybe add SilenceUsage) bools in #169. I thinks it's easier for someone to implement.

@apriendeau
Copy link
Contributor

I can add SilenceUsage today and reimplement the PR. I didn't want to step on this PR, that's why I closed it. Unless @bep wants to do it.

@bep
Copy link
Collaborator Author

bep commented Oct 29, 2015

My use use cases are as follow:

  1. I want to have error handling in one place
  2. I want to show error and usage for certain errors only

The SilenceError may cover my use cases for error logging. SilenceUsage does not, as I see no easy way to know what usage to show from the root command.

@apriendeau
Copy link
Contributor

@bep Can you give an example please? I am trying to understand because the way that I coded it, it should be controlled from each individual command?

@bep
Copy link
Collaborator Author

bep commented Oct 29, 2015

The thing is, I don't know when I create the command what happens in the command's RunE func.

A made up example:

A root command with some stock quote operations:

Command Show quote:

  • Arg Validation failed: show usage + error message "Company symbol must be on the form NNNN".
  • NASDAQ.com is down: Show error.
  • Other error: Behave like today: Usage + error.

@apriendeau
Copy link
Contributor

If thats the case, could you show the usage before you return the error using c.Usage() ?

@bep
Copy link
Collaborator Author

bep commented Oct 29, 2015

Yea, that is the case today. Hugo is sprinkled with c.Usage() -- I want to get rid of those. ===> DRY + they shouldn't need to know the internals of Cobra.

@apriendeau
Copy link
Contributor

Okay cool. We are on the same page now. 👍 So I don't know how I feel about that to be honest. I have mixed feelings because I like the explicit calling of Usage for readability reasons but I can see the case on both sides.

@bep
Copy link
Collaborator Author

bep commented Oct 29, 2015

Yea, programming is a lot about taste :-) Cobra laid the foundation for centralized error handling with the RunE func (without that, it was like, what??), and it would be nice if we could complete that story.

@apriendeau
Copy link
Contributor

It would be good to get a running list of what needs to be complete that. :) I like lists 👍

@bep
Copy link
Collaborator Author

bep commented Oct 29, 2015

I created this pull request as a suggestion. It looks funky, but I have no better suggestion.

The thing is, if a command implements RunE it says: The caller is handling this error. So doing the error logging in this case seems wrong (but changing it may be a breaking change, so make that optional). But then it is the UsageString() method that isn't really available to the caller, so to get the story complete we need some way to signal this usage logging on a error-by-error basis. Hence my two small behavioral interfaces.

@bep
Copy link
Collaborator Author

bep commented Nov 15, 2015

OK; to solve my problem in another way: Is there a way to get the UsageString from the executing command from the root command? So:

if err := rootCmd.Execute(); err != nil {
    // how do I print the UsageString here?
}

@apriendeau
Copy link
Contributor

@bep what if you propagated as part of the error or create custom errors?

@bep
Copy link
Collaborator Author

bep commented Nov 15, 2015

@apriendeau My main point in all of this is that I want my error handling in one place. Having some extra error wrapping logic in all of my commands does not help with that dream.

@apriendeau
Copy link
Contributor

apriendeau commented Nov 15, 2015

I completely agree. I meant that as a solution to get that. I would
implement a custom struct that implements the error interface. The .Error()
function would have to have some logic to add the usage or not, In your
custom type add a way to store the command and a boolean for usage or not.
I can code an example really quick but I want to know if I am way off base
from your idea.

@bep
Copy link
Collaborator Author

bep commented Nov 15, 2015

I understand what you mean, but as I said, it wouldn't help.

@apriendeau
Copy link
Contributor

Okay. So thats my best suggestion because thats what I did and it works for me.

@apriendeau
Copy link
Contributor

@bep I think we are good to close this PR right?

@bep bep closed this Nov 23, 2015
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 this pull request may close these issues.

4 participants