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

JSON output for get and list commands #394

Merged
merged 11 commits into from
Feb 4, 2023
Merged

JSON output for get and list commands #394

merged 11 commits into from
Feb 4, 2023

Conversation

fkorotkov
Copy link
Contributor

In the light of the upcoming 1.0.0 release and stabilizing of the API, let's introduce some breaking changes for the good.

Removed all the --cpu, --memory, --disk and --display flags and replaced with a single --json flag for machine-readable output.

Added --json option to the list command to output a single JSON list. Notably removed --quite flag since it seemed unnecessary.

Fixes #297

In the light of the upcoming `1.0.0` release and stabilizing of the API, let's introduce some breaking changes for the good.

Removed all the `--cpu`, `--memory`, `--disk` and `--display` flags and replaced with a single `--json` flag for machine-readable output.

Added `--json` option to the `list` command to output a single JSON list. Notably removed `--quite` flag since it seemed unnecessary.

Fixes #297
@fkorotkov
Copy link
Contributor Author

FYI @tomjn @fmoc @hokadiri @josephburnett

fkorotkov added a commit to cirruslabs/cirrus-cli that referenced this pull request Feb 2, 2023
@fkorotkov fkorotkov enabled auto-merge (squash) February 3, 2023 02:17
@edigaryev
Copy link
Collaborator

  1. I do think it's beneficial to keep the -q and --quiet for simple use-cases like Delete all local images #375 (comment).

    I would never recommend anyone to parse table-like output that the tart list provides as this will likely break in the future (Machine friendly output #297 (comment)), and processing --json output with jq might be too complicated for this every-day task.

  2. IMO the VM size should be printed in human-understandable units and follow the source and name of the VM. tart list output when running this PR:

    Source Size Name                                                                                                         
    local  19   macos                                                                                                        
    oci    50   ghcr.io/cirruslabs/macos-ventura-base:latest
    

    Proposed tart list output:

    Source Name                                               Size    
    local  macos                                              18.45 GB
    oci    ghcr.io/cirruslabs/macos-ventura-base:latest       46.6 GB
    
  3. The --table/--json Output format (default: table) option looks a bit weird, perhaps simply adding a --json flag should be enough?

  4. There seems to be an extraneous newline emitted at the end when using tart get and tart list.

@tomjn
Copy link

tomjn commented Feb 3, 2023

and processing --json output with jq might be too complicated for this every-day task.

Without jq parsing it in shell script/bash becomes significantly harder, jq makes it much easier. If you're going to add JSON don't piecemeal it just add it everywhere in a consistent manner.

Use of JSON can also be an indicator that the intended recipient is inhuman

IMO the VM size should be printed in human-understandable units and follow the source and name of the VM.

This sounds like a great excuse for a flag/parameter and a new issue+PR

The --table/--json Output format (default: table) option looks a bit weird, perhaps simply adding a --json flag should be enough?

Lets not forget YAML or real ASCII tables ( not pretend TSV's ), why not a format parameter?

WP CLI uses an optional --format parameter that works on all commands

wp post list # the default, same as table
wp post list --format=table
wp post list --format=json
wp post list --format=yaml

It also allows for -q and --quiet.

This is possible because there's a separation between doing things and generating output, with dedicated utilities just for formatting raw data into presentable text

https://make.wordpress.org/cli/handbook/references/internal-api/wp-cli-utils-format-items/

@fmoc
Copy link

fmoc commented Feb 3, 2023

Docker for instance provides pretty-printed JSON when using inspect, which works well enough in the rare occasions you need to read those attributes manually. (With jq, you can even color-format the output, but in my opinion some default formatting/pretty-printing should be included so one does not depend on this tool.)

@fkorotkov
Copy link
Contributor Author

Thank you all for the feedback. I've addressed all of it except human readable disk size for the text output format. IMO for JSON is should be just an Integer value but I haven't found how to make formatting customization for text vs json output.

@edigaryev I've also reverted --quite flag in f1dbe3e. Fixed new line in b836429. PTAL.

@edigaryev edigaryev self-requested a review February 4, 2023 07:40
@fkorotkov fkorotkov merged commit f34aa5f into main Feb 4, 2023
@fkorotkov fkorotkov deleted the json-output branch February 4, 2023 07:40
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.

Machine friendly output
4 participants