kernelci.cli: add echo_json() function #2232

gctucker · 2023-12-08T10:23:12Z

Add kernelci.cli.echo_json() function as a convenience for the common
use-case where some JSON data needs to be dumped on stdout.  Update
all subcommands accordingly to use it.  This takes into account that
indent=0 should mean no formatting, rather than indentation with only
line returns and no spaces.

JenySadadia · 2023-12-08T13:18:27Z

LGTM. Please fix linter errors.

gctucker · 2023-12-11T09:22:47Z

Thanks! Please don't review PRs that are not ready for review though ;)

Add kernelci.cli.echo_json() function as a convenience for the common use-case where some JSON data needs to be dumped on stdout. Update all subcommands accordingly to use it. This takes into account that indent=0 should mean no formatting, rather than indentation with only line returns and no spaces. Signed-off-by: Guillaume Tucker <[email protected]>

JenySadadia · 2023-12-13T08:14:19Z

kernelci/cli/node.py

    attributes = split_attributes(attributes)
    nodes = api.get_nodes(attributes, offset, limit)
-    data = json.dumps(nodes, indent=indent)
+    data = json.dumps(nodes, indent=indent or None)


indent will be set to None by default if not provided.
Also, indent=indent or None would be evaluated to None if user has provided 0 as indent.

Suggested change

data = json.dumps(nodes, indent=indent or None)

data = json.dumps(nodes, indent=indent)

No, I made this change on purpose as otherwise it prints the output formatted with newlines and no identation space when indent=0. So None is needed in this case.

It's also explained in the git commit and the echo_json() docstring.

JenySadadia · 2023-12-13T08:14:48Z

kernelci/cli/user.py

    attributes = split_attributes(attributes)
    users = api.get_users(attributes)
-    data = json.dumps(users, indent=indent)
+    data = json.dumps(users, indent=indent or None)


Same comment as above.

Of course, I fixed this everywhere :) And then wrapped it up in a helper function.

gctucker · 2023-12-13T12:34:55Z

With the indent or None approach as per this PR:

$ ./kci node find name=checkout --api=staging -l1 --indent=0
[{"id": "64b52b7fb5b4a9b8c05e0874", "kind": "node", "name": "checkout", "path": ["checkout"], "group": null, "revision": {"tree": "kernelci", "url": "https://github.com/kernelci/linux.git", "branch": "staging-mainline", "commit": "273cecedf983df3b9cc3fba85b773be154852975", "describe": "staging-mainline-20230717.0", "version": {"version": 6, "patchlevel": 5, "sublevel": null, "extra": "-rc2-1-g273cecedf983", "name": null}, "patchset": null}, "parent": null, "state": "done", "result": null, "artifacts": {"tarball": "http://storage.staging.kernelci.org/api/linux-kernelci-staging-mainline-staging-mainline-20230717.0.tar.gz"}, "data": null, "created": "2023-07-17T11:52:31.913000", "updated": "2023-07-17T12:05:29.780000", "timeout": "2023-07-17T17:52:31.913000", "holdoff": "2023-07-17T12:03:49.144000", "owner": "staging.kernelci.org", "user_groups": []}]

With just indent left to 0 and not converted to None:

$ ./kci node find name=checkout --api=staging -l1 --indent=0
[
{
"id": "64b52b7fb5b4a9b8c05e0874",
"kind": "node",
"name": "checkout",
"path": [
"checkout"
],
"group": null,
"revision": {
"tree": "kernelci",
"url": "https://github.com/kernelci/linux.git",
"branch": "staging-mainline",
"commit": "273cecedf983df3b9cc3fba85b773be154852975",
"describe": "staging-mainline-20230717.0",
"version": {
"version": 6,
"patchlevel": 5,
"sublevel": null,
"extra": "-rc2-1-g273cecedf983",
"name": null
},
"patchset": null
},
"parent": null,
"state": "done",
"result": null,
"artifacts": {
"tarball": "http://storage.staging.kernelci.org/api/linux-kernelci-staging-mainline-staging-mainline-20230717.0.tar.gz"
},
"data": null,
"created": "2023-07-17T11:52:31.913000",
"updated": "2023-07-17T12:05:29.780000",
"timeout": "2023-07-17T17:52:31.913000",
"holdoff": "2023-07-17T12:03:49.144000",
"owner": "staging.kernelci.org",
"user_groups": []
}
]

JenySadadia · 2023-12-13T12:39:40Z

With just indent left to 0 and not converted to None:

$ ./kci node find name=checkout --api=staging -l1 --indent=0
[
{
"id": "64b52b7fb5b4a9b8c05e0874",
"kind": "node",
"name": "checkout",
"path": [
"checkout"
],
"group": null,
"revision": {
"tree": "kernelci",
"url": "https://github.com/kernelci/linux.git",
"branch": "staging-mainline",
"commit": "273cecedf983df3b9cc3fba85b773be154852975",
"describe": "staging-mainline-20230717.0",
"version": {
"version": 6,
"patchlevel": 5,
"sublevel": null,
"extra": "-rc2-1-g273cecedf983",
"name": null
},
"patchset": null
},
"parent": null,
"state": "done",
"result": null,
"artifacts": {
"tarball": "http://storage.staging.kernelci.org/api/linux-kernelci-staging-mainline-staging-mainline-20230717.0.tar.gz"
},
"data": null,
"created": "2023-07-17T11:52:31.913000",
"updated": "2023-07-17T12:05:29.780000",
"timeout": "2023-07-17T17:52:31.913000",
"holdoff": "2023-07-17T12:03:49.144000",
"owner": "staging.kernelci.org",
"user_groups": []
}
]

But isn't it what a user expects with indent=0? I mean this is a normal behavior of json.dumps with indent=0.

gctucker · 2023-12-13T12:45:39Z

It doesn't matter what json.dumps() does, what matters is that when a user wants no indentation there should be no formatting at all. If we want to have the option to actually output the JSON formatted with newlines and no indentation, then I would argue for a special indent value e.g. --indent=newline or something. But I really don't think that's what people would expect. No formatting is standard for storing JSON in a file or machine-to-machine data exchanges, and that's what indent=0 does here. Indentation is useful for human-readable data, and in this case nobody wants an output with less than one space as then it's not really readable.

JenySadadia · 2023-12-14T05:50:15Z

It doesn't matter what json.dumps() does, what matters is that when a user wants no indentation there should be no formatting at all. If we want to have the option to actually output the JSON formatted with newlines and no indentation, then I would argue for a special indent value e.g. --indent=newline or something. But I really don't think that's what people would expect. No formatting is standard for storing JSON in a file or machine-to-machine data exchanges, and that's what indent=0 does here. Indentation is useful for human-readable data, and in this case nobody wants an output with less than one space as then it's not really readable.

Makes sense. Thanks for clarifying.

gctucker · 2023-12-14T08:48:54Z

Thanks for the review :)

gctucker mentioned this pull request Dec 8, 2023

kernelci.cli: use catch_http_error in all API calls #2234

Merged

gctucker force-pushed the cli-echo-json branch from d77b485 to 827d33d Compare December 11, 2023 11:24

gctucker mentioned this pull request Dec 11, 2023

kernelci.cli: use click.File for reading input #2233

Merged

gctucker force-pushed the cli-echo-json branch 2 times, most recently from fede860 to c97d0c0 Compare December 11, 2023 11:46

gctucker force-pushed the cli-echo-json branch from c97d0c0 to 084bea3 Compare December 12, 2023 08:44

gctucker marked this pull request as ready for review December 12, 2023 08:44

JenySadadia reviewed Dec 13, 2023

View reviewed changes

JenySadadia approved these changes Dec 14, 2023

View reviewed changes

JenySadadia added this pull request to the merge queue Dec 14, 2023

Merged via the queue into kernelci:main with commit c4a3c1f Dec 14, 2023

gctucker deleted the cli-echo-json branch December 14, 2023 08:48

	data = json.dumps(nodes, indent=indent or None)
	data = json.dumps(nodes, indent=indent)

kernelci.cli: add echo_json() function #2232

kernelci.cli: add echo_json() function #2232

Uh oh!

Conversation

gctucker commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JenySadadia commented Dec 8, 2023

Uh oh!

gctucker commented Dec 11, 2023

Uh oh!

JenySadadia Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

gctucker Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

gctucker Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

JenySadadia Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gctucker Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

gctucker commented Dec 13, 2023

Uh oh!

JenySadadia commented Dec 13, 2023

Uh oh!

gctucker commented Dec 13, 2023

Uh oh!

JenySadadia commented Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gctucker commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gctucker commented Dec 8, 2023 •

edited

Loading

JenySadadia Dec 13, 2023 •

edited

Loading

JenySadadia commented Dec 14, 2023 •

edited

Loading