[code-cleanup] Rename devbox.Writer to Stderr #1507
Conversation
internal/boxcli/root.go
Outdated
| @@ -49,6 +52,8 @@ func RootCmd() *cobra.Command { | |||
| SilenceErrors: true, | |||
| SilenceUsage: true, | |||
| } | |||
| command.SetOut(os.Stdout) | |||
There was a problem hiding this comment.
maybe I am misunderstanding.
I thought the idea behind cmd.OutOrStdout() was that tests could do command.SetOut and then inspect the output.
If we're overriding here what the tests may set, then it won't work as desired.
There was a problem hiding this comment.
So out is not set by default in cobra. So cmd.OutOrStdout returns stdout because out is not set. But cmd.Print uses OutOrStderr (don't ask me why, I don't understand it) so it prints to stderr.
I'll fix this by removing cmd.Print and replacing with fmt.Fprint(cmd.OutOrStdout()
internal/impl/devbox.go
Outdated
| @@ -92,7 +89,7 @@ func Open(opts *devopt.Opts) (*Devbox, error) { | |||
| nix: &nix.Nix{}, | |||
| projectDir: projectDir, | |||
| pluginManager: plugin.NewManager(), | |||
| writer: opts.Writer, | |||
| Stderr: opts.Stderr, | |||
There was a problem hiding this comment.
does Stderr need to be public?
There was a problem hiding this comment.
good point. Will fix.
| PrintEnvVars(ctx context.Context) ([]string, error) | ||
| PrintGlobalList() error | ||
| NixEnv(ctx context.Context, includeHooks bool) (string, error) | ||
| PackageNames() []string |
There was a problem hiding this comment.
Are these config PackageNames or installable PackageNames?
There was a problem hiding this comment.
Just what is in config. This is a no-op (PrintGlobaList used to call PackageNames).
We can chat about whether this is correct or not (I think it's fine), but would rather not change it in this PR.
internal/boxcli/root.go
Outdated
| @@ -49,6 +52,8 @@ func RootCmd() *cobra.Command { | |||
| SilenceErrors: true, | |||
| SilenceUsage: true, | |||
| } | |||
| command.SetOut(os.Stdout) | |||
There was a problem hiding this comment.
maybe I am misunderstanding.
I thought the idea behind cmd.OutOrStdout() was that tests could do command.SetOut and then inspect the output.
If we're overriding here what the tests may set, then it won't work as desired.
internal/boxcli/integrate.go
Outdated
There was a problem hiding this comment.
In this case either works, but stdErr is logically correct. 👍
Summary
This renames the devbox
Writerfield to the more accurateStderr. It changes a few functions that used to print stuff and now they return strings instead.Also renamed a few functions that where called
PrintXto something more accurate because they don't actually print anything.How to review:
devopt.Optsthis will show all the places where we used to pass in stdout sometimes. Specifically @mohsenari please look at integrate.go. @ipince please look at telemetry.go. I think both of these were wrong.How was it tested?
compiles