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.

Sign up for GitHub

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

Jump to bottom

Consistently redirect usage examples to stdout #1105

Merged
merged 2 commits into from
Aug 31, 2024
Merged

Consistently redirect usage examples to stdout #1105

merged 2 commits into from
Aug 31, 2024

Conversation

tinvaan
Copy link
Contributor

@tinvaan tinvaan commented Aug 29, 2024

Addresses #689

Test plan

Tested manually. Re-directed src command and sub-commands outputs for default usage (no args) and with help/--help args and checked all output was captured.

@tinvaan tinvaan requested a review from a team as a code owner August 29, 2024 04:40
@peterguy
Copy link
Contributor

Sending error and usage output to stderr is common practice for command-line tools because historically many of them were designed for chaining, where the stdout of one could be sent to another, so printing help/usage to stdout could break that chaining.

That's probably why the default channel for FlagSet is stderr.

I don't think that src is designed for chaining, so changing help/usage output to stdout should be ok, but I'll check internally.

@peterguy
Copy link
Contributor

@camdencheek pointed out that even if src is used in chaining (might be for things like src search), sending help/usage to stdout doesn't break that, and errors are still sent to stderr

peterguy
peterguy approved these changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@peterguy peterguy left a comment

Choose a reason for hiding this comment

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

Confirmed that errors are still sent to stderr. Help/usage output is now sent to stdout.

@tinvaan
Copy link
Contributor Author

tinvaan commented Aug 31, 2024

Thanks for the review @peterguy! Can you help merge this changeset? I don't have write access.

@peterguy peterguy merged commit 95b5854 into sourcegraph:main Aug 31, 2024
2 checks passed
@peterguy
Copy link
Contributor

@tinvaan done!

@tinvaan tinvaan deleted the help/stdout branch August 31, 2024 17:02
burmudar pushed a commit to sourcegraph/src-cli-snapshot that referenced this pull request Oct 4, 2024
@tinvaan @burmudar
Consistently redirect usage examples to stdout (sourcegraph#1105)
7833ec1 
* direct help texts to stdout

* set usage output for subcommands
willdollman added a commit that referenced this pull request Oct 15, 2024
@willdollman
Revert "Consistently redirect usage examples to stdout (#1105)"
1652006 
This reverts commit 95b5854.
@willdollman willdollman mentioned this pull request Oct 15, 2024
Merged
willdollman added a commit that referenced this pull request Oct 15, 2024
@willdollman
Fix #1105: Execute commands with zero arguments correctly (#1117)
a9118d5 
* Don't return help for commands with no arguments

* Standarise --help flag across different commands

* Update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterguy peterguy peterguy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tinvaan @peterguy