Normalize printing to Out vs. Err streams

[mislav]

• Have command.Print*() print to stdout, and print errors & warnings to stderr
• Deprecate command.OutOrStderr()
• Handle errors in built-in help command

Warning: backwards-incompatible.

Fixes #1708

[marckhouzam]

Thanks @mislav. Cobra does have some inconsistencies about printing output. However Cobra being so widely used we have to prioritize backward-compatibility at the expense of standardizing things.

If we ever decided to have a Cobra v2, we would surely standardize the output handling. But moving to a new major version had such a large impact to projects that I would personally prefer avoiding it.

[mislav]

@marckhouzam Thanks for chiming in. And I understand if backwards compatibility is a priority, then it's not a good idea to merge this. Feel free to close; this changeset was mostly meant to complement the referenced issue.

[marckhouzam]

Thank you for taking the time @mislav.
I'll check with the other maintainers what we should do with the PR.

[mislav]

Another idea: taking an approach where backwards compatibility is kept, but introducing a new opt-in method that replaces command.SetOut() with a new one that does the right thing. This already happened once in Cobra's history: a method named SetOutput() that handled both out+err streams was deprecated in favor of SetOut() and SetErr().

So for example, we could publish a new method e.g. command.SetOutNonBroken() (final name TBD) and deprecate SetOut in favor of it. What SetOutNonBroken would do is to define the "Out" stream where help text, completion, and version information are written; but keep writing errors and warning messages to the "Err" stream.

[jpmcb]

Marking myself for review.

---

I agree - there are some major inconsistencies across many of Cobra's APIs.

I'm unsure what to do; the number of things we'd like to fix that would be a breaking change in Cobra could have wide reaching impacts across kubernetes, enterprise software, GitHub, etc.

But these are improvements we should make. And the maintainers should to some degree feel empowered to make these changes.

TLDR: I'd like to do this. And I think it should exist in some new 2.0 of Cobra, I'm just unsure how to start moving towards that.

[mislav]

Thanks for taking the time to chime in. What is the most conservative change that the maintainers of Cobra would be willing to accept in order to fix the main bug in the referenced issue (deprecation warnings going to the "Out" stream)?

I'm thinking something along these lines: avoid changing the functionality of cmd.Print*(), in case Cobra apps depend on those, but change the internal logic that prints warnings (e.g. deprecation messages) to go straight to ErrOrStderr() instead of using cmd.Print*(). This would technically break apps that depend on deprecation warnings being written to the "Out" stream, but I don't suspect anyone actually relies on that functionality in their apps?

[github-actions]

The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the PR is closed.
  You can:
• Make a comment to remove the stale label and show your support. The 60 days reset. - If a PR has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interseted in reopening.

[github-actions]

The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the PR is closed.
  You can:
• Make a comment to remove the stale label and show your support. The 60 days reset. - If a PR has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interseted in reopening.

[cbandy]

I'm still interested, though the attached issue has lapsed.

[mislav]

Per my previous comment, I'm still willing to do this:

What is the most conservative change that the maintainers of Cobra would be willing to accept in order to fix the main bug in the referenced issue (deprecation warnings going to the "Out" stream)?

But I'd like a confirmation from Cobra core team (or person) before I proceed, because I wouldn't like to continue doing work on this if it's going to just get closed by the stale bot (like the referenced issue I opened).

My idea would be to trim down this PR to just bug fixes (e.g. deprecation notices go to "err" stream instead of "out" stream) but leave public APIs the same (i.e. cmd.Print*() set of functions remains unchanged).

[github-actions]

The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the PR is closed.
  You can:
• Make a comment to remove the stale label and show your support. The 60 days reset. - If a PR has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interested in reopening.

[tcarreira]

I'm interested as well.
I support implementing non-breaking changes, and I believe it can be accomplished if the maintainers give their approval.
@jpmcb, what are your thoughts on this?

There is one specific task I'd like to perform in my app: adding unit tests to verify the output of stdout/stderr.
However, I've noticed that using the SetOut/SetErr methods consistently produces inconsistent results."

[mislav]

The referenced issue got re-opened and in the meantime this PR got outdated, so I'm closing it in favor of the discussion in the issue. There still doesn't seem to be consensus in how this should be solved, so until there is, I don't think there needs to exist a PR that proposes code changes.

[jpmcb]

Thanks for the contribution @mislav and apologies we couldn't get this in.

Yes, let's continue the discussion in the issue.