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
bpo-29636: Add --indent / --no-indent arguments to json.tool #345
Conversation
Note to rebase on #346 which should be merged first. |
group.add_argument('--indent', default=4, type=int, | ||
help='Indent level (number of spaces) for ' | ||
'pretty-printing. Defaults to 4.') | ||
group.add_argument('--no-indent', action='store_const', dest='indent', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a --compact option which use indent=None but also separators without spaces: http://bugs.python.org/issue29540
It's not the same than what you propose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Haypo I agree the most compact representation would be useful.
One option would be add a third argument (--compact
) to the mutually exclusive group. This would provide access to three options:
--indent
: pretty (with newlines)--no-indent
: withindent=None
injson.dump
--compact
: compact, withseparators=(',', ':')
injson.dump
Alternatively, are you proposing to only support options 1 & 3?
I'm happy to implement this. Let's get a second opinion, so I don't go down the wrong track. Tagging @serhiy-storchaka, @berkerpeksag, @methane.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is --no-indent differen than --indent=0?
Would it be possible to use a default of 4 spaces when using --indent with no argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is --no-indent differen than --indent=0?
Yes, it is. --indent=0
adds new lines, --no-indent
writes all in one line.
Would it be possible to use a default of 4 spaces when using --indent with no argument?
This is the default behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Haypo the following illustrates the three options
import json
obj = [1, 2]
print('--indent=4')
print(json.dumps(obj, indent=4))
print('--no-indent')
print(json.dumps(obj, indent=None))
print('--compact')
print(json.dumps(obj, separators=(',', ':')))
which produces the following output
--indent=4
[
1,
2
]
--no-indent
[1, 2]
--compact
[1,2]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spaces after comma and semicolon are not just for readability. They make JSON compatible with YAML 1.0.
@serhiy-storchaka great point. I think this is a strong argument for not replacing --no-indent
with --compact
. However, should we add --compact
as a third option? I can see the utility for users who wan't the smallest file sizes.
Spaces after comma and semicolon are not just for readability. They make JSON compatible with YAML 1.0. |
Yes, it is. --indent=0 adds new lines, --no-indent writes all in one line.
"write all in one line": it sounds like the use case is to produce
smaller file, so it seems redundant and less efficient than --compact.
I expect that --compact removes all spaces and newlines to produce the
smallest file.
|
This is not the right place for the design discussion. Lets continue it on the bug tracker. |
@serhiy-storchaka and @Haypo I updated bpo-29636 with a summary of the design discussion thus far. I propose continuing this pull request with only |
#346 has been merged and this PR rebased. @serhiy-storchaka, @berkerpeksag, @methane and @Haypo: open for review. No more outstanding changes on my end. |
b53a918
to
56a3851
Compare
Originall from python#2720 Set default option for infile and outfile as per python#2720 (review) Use --sort-keys help message based on the json.tool docs at https://docs.python.org/3.6/library/json.html#basic-usage: Remove commens as per https://bugs.python.org/msg298692. Code was descriptive without the comments.
From python#201 which is being split into two pull requests. See http://bugs.python.org/issue29636
@dhimmel Please add NEWS entry. |
Thanks @methane for ba16891 which merges in the changes from #5315 (dict order guaranteed as of Python 3.7). I'll get working on the NEWS entry. |
Use `blurb add` command to assist with adding the NEWS entry.
Ready from my end. As of b8fee34,
|
What if pass conflicting options? |
Lib/json/tool.py
Outdated
@@ -21,23 +21,42 @@ def main(): | |||
'to validate and pretty-print JSON objects.') | |||
parser = argparse.ArgumentParser(prog=prog, description=description) | |||
parser.add_argument('infile', nargs='?', type=argparse.FileType(), | |||
default=sys.stdin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of setting default to sys.stdin
instead of keeping it None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way a suggestion from another contributor at #2720 (comment):
it is easier to read when we process all options in one place, and then use the process options.
By adding default=sys.stdin
, we remove the following line later on:
infile = options.infile or sys.stdin
Lib/json/tool.py
Outdated
parser.add_argument('--sort-keys', action='store_true', default=False, | ||
help='sort the output of dictionaries alphabetically by key') | ||
parser.add_argument('--sort-keys', action='store_true', | ||
help='sort the output of dictionaries by key') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removed "alphabetically"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe my intention was to make the help message more closely mirror the json.dump
documentation:
Lines 157 to 158 in cfa797c
If *sort_keys* is true (default: ``False``), then the output of | |
dictionaries will be sorted by key. |
Happy to re-add "alphabetically" if that was helpful.
The intention is to disallow users from passing multiple whitespace options with argparse's For example,
|
I hope this feature can be merged ASAP, it's really helpful as a cross-platform command line utility. |
@mya12321 the most effective way for you to support this PR would be to chime in on the issue. At present, I think there is some hesitancy to add additional arguments, so if you explain your use case that may be helpful. But as far as I understand, this discussion should go on the bug tracker and not on GitHub. |
Would you resolve the conflict? |
Well done @dhimmel and thanks for your tenacity! |
Bumps [gidgethub](https://github.com/brettcannon/gidgethub) from 4.1.0 to 4.1.1. - [Release notes](https://github.com/brettcannon/gidgethub/releases) - [Changelog](https://github.com/brettcannon/gidgethub/blob/master/docs/changelog.rst) - [Commits](gidgethub/gidgethub@v4.1.0...v4.1.1) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
From #201 which is being split into two pull requests.
Tagging @serhiy-storchaka, @berkerpeksag, @methane who helped review #201.
Closes http://bugs.python.org/issue29636.
https://bugs.python.org/issue29636