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 documentation of Command.SetOut method #2037

Closed
wants to merge 1 commit into from

Conversation

kanaksinghal
Copy link

@kanaksinghal kanaksinghal commented Oct 4, 2023

This change fixes a misleading documentation issue for methods: SetOut, SetErr, SetIn & SetOutput

In Command.SetOut, the statement "If newOut is nil, os.Stdout is used" was misleading as Command.Print prints to os.Stderr when outWriter = nil

The defaults are based on the getter being used i.e. one of OutOrStdout or OutOrStderr. i.e. there is no default.

@CLAassistant
Copy link

CLAassistant commented Oct 4, 2023

CLA assistant check
All committers have signed the CLA.

The statement "If newOut is nil, os.Stdout is used" was misleading as
`Command.Print` prints to os.Stderr when `outWriter = nil`

As the default is based on which one of `OutOrStdout` or `OutOrStderr`
is used, i.e. there is no default.
@marckhouzam
Copy link
Collaborator

I agree that there is an inconsistency here. I also feel the use of OutOrStderr by Print() is a bug we are stuck with due to backwards-compatibility.

Now, if you don't call SetOut() or call it with nil, the existing printouts of cobra will default to stdout, so the comment feels right to me. If you add printout using Pint() then you justifiably get confused, but there isn't much we can do about it.

So I don't think changing the comment will clarify things. That is why the comment for Print() mention stderr.

@kanaksinghal
Copy link
Author

I see, thanks for clarifying 🙂 👍

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants