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

Fix #1056 #1062

Closed
wants to merge 1 commit into from
Closed

Fix #1056 #1062

wants to merge 1 commit into from

Conversation

ob-stripe
Copy link

Restore previous behavior for additional help topic commands.

Fixes #1056.

cc @bruceadowns

@CLAassistant
Copy link

CLAassistant commented Mar 18, 2020

CLA assistant check
All committers have signed the CLA.

@jharshman jharshman added the kind/bug A bug in cobra; unintended behavior label Mar 26, 2020
@jharshman jharshman requested a review from n10v March 26, 2020 18:29
@jharshman
Copy link
Collaborator

@ob-stripe - thanks for finding this bug. Quick question, what do you think of just reverting to the behavior that we had before #922 ?

@ob-stripe
Copy link
Author

Good question. It's debatable whether not providing a subcommand to a command that expects one should return an error or not.

I'm one of the maintainers of the Stripe CLI which makes heavy use of nested subcommands. For example, running this:

$ stripe charges
Usage:
  stripe charges <operation> [parameters...]

Available Operations:
  capture
  create
  list
  retrieve
  update

...

For our case at least, I think simply displaying the help message and exiting with status code 0 is preferable. The charges command did its job: display the help message listing available subcommands.

If that's an option, I would prefer reverting to the previous behavior entirely. Maybe we can have the best of both worlds by making the new behavior available on demand?

@jharshman
Copy link
Collaborator

I'm reviewing the issues that brought the change in #922 about. From what I can tell, it was a debate about whether or not Cobra should emit an exit code for a command that doesn't exist.

My preference here is to lean on RunE. To handle the return of an error code. From my experience, this handles the case nicely.

Since this change really just landed in 0.0.6, I'm definitely okay reverting it.

@ob-stripe
Copy link
Author

Awesome. Feel free to close this PR then.

@jharshman
Copy link
Collaborator

closing in favor of #1068

@jharshman jharshman closed this Mar 26, 2020
@ob-stripe ob-stripe deleted the ob-fix-1056 branch March 27, 2020 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in cobra; unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional help topic commands now return "subcommand is required" error
3 participants