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

Set proper default command output #8628

Merged
merged 13 commits into from
Apr 17, 2021
Merged

Set proper default command output #8628

merged 13 commits into from
Apr 17, 2021

Conversation

RiccardoM
Copy link
Contributor

Description

This PR sets the proper default command output

closes: #8498

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

amaury1093
amaury1093 reviewed Feb 18, 2021
View reviewed changes
simapp/simd/cmd/root.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 18, 2021 •
edited
Loading

Codecov Report

Merging #8628 (1ffa4ae) into master (82dc008) will increase coverage by 0.03%.
The diff coverage is 50.00%.

❗ Current head 1ffa4ae differs from pull request most recent head 9a60bda. Consider uploading reports for the commit 9a60bda to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8628      +/-   ##
==========================================
+ Coverage   58.79%   58.83%   +0.03%     
==========================================
  Files         583      580       -3     
  Lines       32752    32636     -116     
==========================================
- Hits        19257    19200      -57     
+ Misses      11218    11170      -48     
+ Partials     2277     2266      -11
Impacted Files Coverage Δ
client/flags/flags.go 22.22% <ø> (+1.81%) ⬆️
client/keys/add.go 62.50% <ø> (-0.45%) ⬇️
client/keys/list.go 72.22% <ø> (-1.47%) ⬇️
client/keys/migrate.go 56.33% <0.00%> (ø)
version/command.go 83.33% <ø> (-0.67%) ⬇️
x/auth/client/cli/tx_sign.go 73.74% <ø> (-0.15%) ⬇️
x/auth/client/cli/validate_sigs.go 79.71% <0.00%> (ø)
x/feegrant/types/filtered_fee.go 0.00% <ø> (ø)
simapp/simd/cmd/root.go 62.30% <100.00%> (+0.58%) ⬆️
types/coin.go 93.11% <0.00%> (-0.16%) ⬇️
... and 7 more

amaury1093
amaury1093 reviewed Feb 18, 2021
View reviewed changes
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm, though I didn't test it. @RiccardoM did you test it locally on your machine?

Also, a changelog entry would be nice. I believe this is considered client-breaking.

@RiccardoM
Copy link
Contributor Author

RiccardoM commented Feb 18, 2021 •
edited
Loading

lgtm, though I didn't test it. @RiccardoM did you test it locally on your machine?

Also, a changelog entry would be nice. I believe this is considered client-breaking.

Added the CHANGELOG entry.

I also moved the place where the outputs were initialized cause it didn't work. The new method using the already existing PersistentPreRunE instead of PersistentPreRun now works properly.

alessio
alessio suggested changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Hold off please. Errors and diagnostics always go to stderr. When a command is meant to produce an output, that should go to stdout. Don't change defaults please.

@RiccardoM
Copy link
Contributor Author

Hold off please. Errors and diagnostics always go to stderr. When a command is meant to produce an output, that should go to stdout. Don't change defaults please.

Shouldn't this be a responsibility of commands? IMO they should call cmd.PrintErrln() for errors, not cmd.Println. It would make more sense from my point of view to use the proper methods instead of having the default output to stderr

@alessio
Copy link
Contributor

alessio commented Feb 18, 2021

Hold off please. Errors and diagnostics always go to stderr. When a command is meant to produce an output, that should go to stdout. Don't change defaults please.

IMO they should call cmd.PrintErrln() for errors, not cmd.Println.

Yes, I agree. It makes sense. I agree with @alexanderbez, we need some grooming here. Just changing defaults seems the most obvious way to get the wrong solution in place.

@michaelfig
Copy link
Contributor

It is unfortunate that this PR has not gained any traction. It is a major ergonomic failure (and violation of standard *nix pipelineable commands) to have machine-readable output go to stderr. If I run appd status | jq ..., the output is not captured. If I run appd status 2>&1 | jq ... then error messages are combined with the output and are hidden from view if there is a failure.

@alessio, what will it take to merge?

@alessio
Copy link
Contributor

alessio commented Apr 9, 2021

It is unfortunate that this PR has not gained any traction. It is a major ergonomic failure (and violation of standard *nix pipelineable commands) to have machine-readable output go to stderr. If I run appd status | jq ..., the output is not captured. If I run appd status 2>&1 | jq ... then error messages are combined with the output and are hidden from view if there is a failure.

@alessio, what will it take to merge?

I'm testing this again now.

@alessio alessio self-assigned this Apr 9, 2021
@alessio
Copy link
Contributor

alessio commented Apr 9, 2021

@RiccardoM mind resolving the conflicts, please?

@RiccardoM
Merge branch 'master' of github.com:cosmos/cosmos-sdk into command-ou…
b855a89 
…tout-fix

� Conflicts:
�	CHANGELOG.md
�	client/keys/migrate.go
�	simapp/simd/cmd/root.go
@RiccardoM
Ran make format
75c2a08
@RiccardoM
Copy link
Contributor Author

@RiccardoM mind resolving the conflicts, please?

Should be done

@alessio alessio mentioned this pull request Apr 13, 2021
Closed
@amaury1093
Copy link
Contributor

tACK. Tested with a couple of CLI subcommands outputting JSON, and piping to jq

@amaury1093
Copy link
Contributor

I believe backporting this could be useful for gaia CLI users.

@amaury1093 amaury1093 added A:automerge Automatically merge PR once all prerequisites pass. backport/0.42.x (Stargate) labels Apr 14, 2021
shahankhatch
shahankhatch approved these changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@shahankhatch shahankhatch left a comment

Choose a reason for hiding this comment

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

Thanks! Very helpful for gaia users.

@michaelfig michaelfig mentioned this pull request Apr 16, 2021
Closed
@mergify mergify bot merged commit 96fe999 into cosmos:master Apr 17, 2021
@michaelfig
Copy link
Contributor

Thank you, everybody!

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
@RiccardoM @michaelfig @amaury1093
Set proper default command output (cosmos#8628)
7eda745 
* Set proper default command output

* Removed duplicated cmd.SetErr(cmd.ErrOrStderr()) and cmd.SetOut(cmd.OutOrStdout())

* Moved command initialization and added CHANGELOG

* fix: groom all uses of cmd.Print*

* Ran make format

Co-authored-by: Michael FIG <[email protected]>
Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Amaury <[email protected]>
@RollkitBot RollkitBot mentioned this pull request Aug 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelfig michaelfig michaelfig left review comments

@shahankhatch shahankhatch shahankhatch approved these changes

@amaury1093 amaury1093 amaury1093 approved these changes

@alessio alessio alessio approved these changes

@aaronc aaronc Awaiting requested review from aaronc

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

Assignees

@alessio alessio

Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong command output
5 participants
@RiccardoM @alessio @michaelfig @amaury1093 @shahankhatch