-
Notifications
You must be signed in to change notification settings - Fork 202
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
[CLI] Quiet mode for all commands #431
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good, but unless I'm misunderstanding something everything should be stdout instead of stderr right?
Side note, we are currently sending errors to stdout in DebugMiddleware
. That's another separate fix but I assume those should go to stderr instead and IMO those should not listen to quiet flag?
func addCmdFunc(_ *cobra.Command, args []string, flags addCmdFlags) error { | ||
box, err := devbox.Open(flags.config.path, os.Stdout) | ||
func addCmdFunc(cmd *cobra.Command, args []string, flags addCmdFlags) error { | ||
box, err := devbox.Open(flags.config.path, cmd.ErrOrStderr()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be OutOrStdout
no?
@@ -26,6 +27,11 @@ func RootCmd() *cobra.Command { | |||
command := &cobra.Command{ | |||
Use: "devbox", | |||
Short: "Instant, easy, predictable development environments", | |||
PersistentPreRun: func(cmd *cobra.Command, args []string) { | |||
if flags.quiet { | |||
cmd.SetErr(io.Discard) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be SetOut
?
PersistentPreRunE: nix.EnsureInstalled, | ||
Use: "add <pkg>...", | ||
Short: "Add a new package to your devbox", | ||
PreRunE: nix.EnsureInstalled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why this change? can we not have 2 PersistentPreRunE
? (one in root and another in the subcommands?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A found it spf13/cobra#252
looks like others were confused by this as well. Good find!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was causing the subcommands ignore the persistentPreRun at root level which contains -q
flag. This was the bug I couldn't solve yesterday but eventually did 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found Slack conversation where stderr vs out was discussed. IMakes sense
Also found this https://unix.stackexchange.com/questions/331611/do-progress-reports-logging-information-belong-on-stderr-or-stdout
806cc5a
to
1c50665
Compare
1c50665
to
d88f07a
Compare
Summary
PersistentPreRunE
to 'PreRunEin subcommands because it was causing the subcommands ignore the root command's
PersistentPreRunE` which has the quiet flag.How was it tested?