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

Cli: enable json output #9478

Merged
merged 12 commits into from
Apr 14, 2020

Conversation

CriesofCarrots
Copy link
Contributor

Problem

Some of the cli commands do a lot of client-side processing that make them valuable for users, but the displays are geared toward human eyes only. It is very difficult to ingest this data programmatically.

Summary of Changes

  • Add global --output format arg, to set OutputFormat -- currently supports Display (default) and Json
  • Implement json output for various process-heavy cli subcommands. All of these subcommands feature ad-hoc printing, so in order to make json serialization easy and data-maintenance sane, collect the data to return in a Cli-specific struct. Then impl Display for the struct to collect/contain human-readable print formatting.
    As a result, once the struct is populated, output format-selection and printing is triggered with a single line:
    OutputFormat.formatted_print(&<STRUCT>);

Fixes #7827

@CriesofCarrots
Copy link
Contributor Author

@mvines , I would love your feedback on this approach. It's quite a bit of churn, but seems reasonably maintainable, given all the special-case printing going on in the cli.

@codecov
Copy link

codecov bot commented Apr 14, 2020

Codecov Report

Merging #9478 into master will decrease coverage by 0.4%.
The diff coverage is 2.2%.

@@           Coverage Diff            @@
##           master   #9478     +/-   ##
========================================
- Coverage    81.2%   80.7%   -0.5%     
========================================
  Files         277     278      +1     
  Lines       62413   62751    +338     
========================================
+ Hits        50682   50683      +1     
- Misses      11731   12068    +337     

@mvines
Copy link
Member

mvines commented Apr 14, 2020

This approach looks great. I like that we'll be able to more easily unit test the business logic of each command now as well.

Probably want to get this landed ASAP to avoid rebase hell. I'd backport to v1.1 to keep us sane through the end of May, but skip the v1.0 backport since that'll only live for another couple weeks

t-nelson
t-nelson previously approved these changes Apr 14, 2020
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

Looks great! Should be pretty easy to maintain 🙏

All I've got is a feature request 🙂

cli/src/cli_output.rs Show resolved Hide resolved
@mergify mergify bot dismissed t-nelson’s stale review April 14, 2020 17:44

Pull request has been modified.

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Apr 14, 2020
@solana-grimes solana-grimes merged commit 5298e38 into solana-labs:master Apr 14, 2020
mergify bot pushed a commit that referenced this pull request Apr 14, 2020
automerge

(cherry picked from commit 5298e38)

# Conflicts:
#	Cargo.lock
#	cli/Cargo.toml
#	cli/src/lib.rs
#	cli/src/stake.rs
CriesofCarrots added a commit that referenced this pull request Apr 14, 2020
automerge

(cherry picked from commit 5298e38)
solana-grimes pushed a commit that referenced this pull request Apr 14, 2020
@CriesofCarrots CriesofCarrots deleted the cli-json-struct branch April 16, 2020 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make cli commands support emitting JSON
4 participants