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

Print usage for CLI parsing errors #165

Merged
merged 1 commit into from
Oct 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/pkg/cli/analytics.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ func newAnalyticsCommand() *cobra.Command {
return &cobra.Command{
Use: "analytics <on|off>",
Short: "Enable or disable analytics",
RunE: analyticsCommand,
Run: handleErrors(analyticsCommand),
Args: cobra.ExactArgs(1),
}
}
Expand Down
2 changes: 1 addition & 1 deletion cli/pkg/cli/checkout.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func newCheckoutCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "checkout <experiment or checkpoint ID>",
Short: "Copy files from an experiment or checkpoint into the project directory",
RunE: checkoutCheckpoint,
Run: handleErrors(checkoutCheckpoint),
Args: cobra.ExactArgs(1),
}

Expand Down
12 changes: 12 additions & 0 deletions cli/pkg/cli/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,15 @@ func getStorage(storageURL, projectDir string) (storage.Storage, error) {
}
return store, nil
}

// handlErrors wraps a cobra function, and will print and exit on error
//
// We don't use RunE because if that returns an error, Cobra will print usage.
// That behavior can be disabled with SilenceUsage option, but then Cobra arg/flag errors don't display usage. (sigh)
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes I remember this, it's why I turned off usage errors in the first place!

func handleErrors(f func(cmd *cobra.Command, args []string) error) func(cmd *cobra.Command, args []string) {
return func(cmd *cobra.Command, args []string) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that we print usage info on every error? As a future improvement it'd be nice to only print usage info if the user is doing something stupid (not on random internal errors, etc.). Not trivial to implement, but nice to have.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the whole point of this function - it catches errors so cobra doesn’t display them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe not obvious that fatal quits, so I’ll clarify comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, the comment does say that. I guess you misunderstood this, or maybe I'm misunderstanding what you're saying? Do you mean random internal errors in Cobra?

if err := f(cmd, args); err != nil {
console.Fatal(err.Error())
}
}
}
2 changes: 1 addition & 1 deletion cli/pkg/cli/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func newDiffCommand() *cobra.Command {
Long: `Compare two experiments or checkpoints.

If an experiment ID is passed, it will pick the best checkpoint from that experiment. If a primary metric is not defined in replicate.yaml, it will use the latest checkpoint.`,
RunE: diffCheckpoints,
Run: handleErrors(diffCheckpoints),
Args: cobra.ExactArgs(2),
}

Expand Down
2 changes: 1 addition & 1 deletion cli/pkg/cli/feedback.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ func newFeedbackCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "feedback",
Short: "Submit feedback to the team!",
RunE: submitFeedback,
Run: handleErrors(submitFeedback),
}

return cmd
Expand Down
2 changes: 1 addition & 1 deletion cli/pkg/cli/generate_docs.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ func newGenerateDocsCommand(rootCmd *cobra.Command) *cobra.Command {
cmd := &cobra.Command{
Use: "generate-docs",
Short: "",
RunE: generateDocs(rootCmd),
Run: handleErrors(generateDocs(rootCmd)),
Args: cobra.ExactArgs(0),
Hidden: true,
}
Expand Down
2 changes: 1 addition & 1 deletion cli/pkg/cli/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ func newListCommand() *cobra.Command {
Use: "ls",
Short: "List experiments in this project",
Aliases: []string{"list"},
RunE: listExperiments,
Run: handleErrors(listExperiments),
Args: cobra.NoArgs,
}

Expand Down
2 changes: 1 addition & 1 deletion cli/pkg/cli/ps.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ func newPsCommand() *cobra.Command {
Use: "ps",
Short: "List running experiments in this project",
Aliases: []string{"processes"},
RunE: listRunningExperiments,
Run: handleErrors(listRunningExperiments),
Args: cobra.NoArgs,
}

Expand Down
2 changes: 1 addition & 1 deletion cli/pkg/cli/rm.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ func newRmCommand() *cobra.Command {

To remove experiments or checkpoints, pass any number of IDs (or prefixes).
`,
RunE: removeExperimentOrCheckpoint,
Run: handleErrors(removeExperimentOrCheckpoint),
Args: cobra.MinimumNArgs(1),
Aliases: []string{"delete"},
SuggestFor: []string{"remove"},
Expand Down
4 changes: 2 additions & 2 deletions cli/pkg/cli/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ func NewRootCommand() (*cobra.Command, error) {

To learn how to get started, go to ` + global.WebURL + `/docs/tutorial`,

Version: global.Version,
Version: global.Version,
// This stops errors being printed because we print them in cmd/replicate/main.go
SilenceErrors: true,
SilenceUsage: true,
PersistentPreRun: func(cmd *cobra.Command, args []string) {
if global.Verbose {
console.SetLevel(console.DebugLevel)
Expand Down
4 changes: 2 additions & 2 deletions cli/pkg/cli/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ func newRunCommand() *cobra.Command {
Use: "run [flags] <command> [arg...]",
Short: "Run a command on a remote machine",
Args: cobra.MinimumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
Run: handleErrors(func(cmd *cobra.Command, args []string) error {
return runCommand(opts, args)
},
}),
}

flags := cmd.Flags()
Expand Down
2 changes: 1 addition & 1 deletion cli/pkg/cli/show.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func newShowCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "show <experiment or checkpoint ID>",
Short: "View information about an experiment or checkpoint",
RunE: show,
Run: handleErrors(show),
Args: cobra.ExactArgs(1),
}

Expand Down