8000 proposal: better error handling for the cli by nikonov1101 · Pull Request #1152 · sonm-io/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

proposal: better error handling for the cli #1152

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

Merged
merged 1 commit into from
Jul 3, 2018

Conversation

nikonov1101
Copy link
Member
@nikonov1101 nikonov1101 commented Jun 30, 2018

This commit changes CLI behavior in the following way:

  1. Using RunE instead of Run for each cobra command.
  2. Setting SilenceUsage = true, if not - cli will print help usage on every error.
  3. Setting SilenceErrors = true, if not - cli will show their own error and then handle out custom code that controls the os.Exit status.
  4. Introducing newCmd and newCmdWithKeys wrappers. The first one is used to wrapping cobra.Command with SilenceUsage and SilenceErrors flags. The second one is used to wrapping command with the key loading code (if required and not handled externally).
  5. Removed all of os.Exit(1) calls, excepts only one that handles single point of error handling now.

@3Hren
Copy link
Member
3Hren commented Jul 2, 2018

Good. Do more.

Copy link
Collaborator
@antmat antmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to check for error on the upper level and exit with non-zero status code

@nikonov1101 nikonov1101 force-pushed the proposal/better-error-handing-for-cli branch 3 times, most recently from fbc1e38 to 655342c Compare July 3, 2018 14:37
@nikonov1101 nikonov1101 added S: CLI This PR/Issue changes CLI P: low This PR/Issue has minor priority V: patch This PR/Issue requires patch version to be bumped T: experiment This PR/Issue is an experiment T: refactor This PR/Issue refactors some parts of code labels Jul 3, 2018
@nikonov1101 nikonov1101 force-pushed the proposal/better-error-handing-for-cli branch 2 times, most recently from 25b3471 to c7101b4 Compare July 3, 2018 15:49
This commit changes CLI behavior in the following way:
1. Using RunE instead of Run for each cobra command.
2. Setting SilenceUsage = true, if not - cli will print help usage on every error.
3. Setting SilenceErrors = true, if not - cli will show their own error and then handle out custom code that controls the os.Exit status.
4. Introducing `newCmd` and `newCmdWithKeys` wrappers. The first one is used to wrapping cobra.Command with `SilenceUsage` and  `SilenceErrors` flags. The second one is used to wrapping command with the key loading code (if required and not handled externally).
5. Removed all of `os.Exit(1)` calls, excepts only one that handles single point of error handling now.
Short: "Manage blacklisted addresses",
Use: "blacklist",
Short: "Manage blacklisted addresses",
PersistentPostRunE: loadKeyStoreIfRequired,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

img

@nikonov1101 nikonov1101 force-pushed the proposal/better-error-handing-for-cli branch from c7101b4 to d7b5b97 Compare July 3, 2018 15:52
Copy link
Collaborator
@antmat antmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WAH, SLUSHAY!

@antmat antmat merged commit 9229607 into master Jul 3, 2018
@antmat antmat deleted the proposal/better-error-handing-for-cli branch July 3, 2018 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P: low This PR/Issue has minor priority S: CLI This PR/Issue changes CLI T: experiment This PR/Issue is an experiment T: refactor This PR/Issue refactors some parts of code V: patch This PR/Issue requires patch version to be bumped
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0