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

bpo-29636: Add --indent / --no-indent arguments to json.tool #345

Merged
merged 8 commits into from Dec 4, 2019

Conversation

@dhimmel
Copy link
Contributor

@dhimmel dhimmel commented Feb 27, 2017

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

@dhimmel
Copy link
Contributor Author

@dhimmel dhimmel commented Feb 27, 2017

Note to rebase on #346 which should be merged first.

Loading

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',
Copy link
Member

@vstinner vstinner Mar 2, 2017

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.

Loading

Copy link
Contributor Author

@dhimmel dhimmel Mar 3, 2017

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:

  1. --indent: pretty (with newlines)
  2. --no-indent: with indent=None in json.dump
  3. --compact: compact, with separators=(',', ':') in json.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.

Loading

Copy link
Member

@vstinner vstinner Mar 3, 2017

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?

Loading

Copy link
Member

@serhiy-storchaka serhiy-storchaka Mar 3, 2017

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.

Loading

Copy link
Contributor Author

@dhimmel dhimmel Mar 3, 2017

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]

Loading

Copy link
Contributor Author

@dhimmel dhimmel Mar 3, 2017

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.

Loading

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Mar 3, 2017

Spaces after comma and semicolon are not just for readability. They make JSON compatible with YAML 1.0.

Loading

@vstinner
Copy link
Member

@vstinner vstinner commented Mar 3, 2017

Loading

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Mar 3, 2017

This is not the right place for the design discussion. Lets continue it on the bug tracker.

Loading

@dhimmel
Copy link
Contributor Author

@dhimmel dhimmel commented Mar 3, 2017

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 --indent and --no-indent. If we end up reaching consensus to add --compact, that can be added in the future. I think it would fit in nicely alongside --indent and --no-indent. In other words, it's not pressing to decide on --compact right now.

Loading

@dhimmel
Copy link
Contributor Author

@dhimmel dhimmel commented Mar 16, 2017

#346 has been merged and this PR rebased.

@serhiy-storchaka, @berkerpeksag, @methane and @Haypo: open for review. No more outstanding changes on my end.

Loading

dhimmel added 4 commits Jul 20, 2017
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.
@methane
Copy link
Member

@methane methane commented Feb 22, 2018

@dhimmel Please add NEWS entry.

Loading

@dhimmel
Copy link
Contributor Author

@dhimmel dhimmel commented Feb 22, 2018

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.

Loading

dhimmel added 2 commits Feb 22, 2018
Use `blurb add` command to assist with adding the NEWS entry.
@dhimmel
Copy link
Contributor Author

@dhimmel dhimmel commented Feb 22, 2018

Ready from my end.

As of b8fee34, --help prints the following message:

usage: python -m json.tool [-h] [--sort-keys]
                           [--indent INDENT | --tab | --no-indent | --compact]
                           [infile] [outfile]

A simple command line interface for json module to validate and pretty-print
JSON objects.

positional arguments:
  infile           a JSON file to be validated or pretty-printed
  outfile          write the output of infile to outfile

optional arguments:
  -h, --help       show this help message and exit
  --sort-keys      sort the output of dictionaries by key
  --indent INDENT  separate items with newlines and use this number of spaces
                   for indentation
  --tab            separate items with newlines and use tabs for indentation
  --no-indent      separate items with spaces rather than newlines
  --compact        suppress all whitespace separation (most compact)

Loading

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Feb 22, 2018

What if pass conflicting options? --indent=4 and --tab? --indent=4 and --compact?

Loading

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,
Copy link
Member

@serhiy-storchaka serhiy-storchaka Feb 22, 2018

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?

Loading

Copy link
Contributor Author

@dhimmel dhimmel Feb 22, 2018

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

Loading

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')
Copy link
Member

@serhiy-storchaka serhiy-storchaka Feb 22, 2018

Choose a reason for hiding this comment

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

Why removed "alphabetically"?

Loading

Copy link
Contributor Author

@dhimmel dhimmel Feb 22, 2018

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:

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.

Loading

@dhimmel
Copy link
Contributor Author

@dhimmel dhimmel commented Feb 22, 2018

What if pass conflicting options? --indent=4 and --tab? --indent=4 and --compact?

The intention is to disallow users from passing multiple whitespace options with argparse's add_mutually_exclusive_group. The implementation will, in most cases, detect and raise an error if multiple whitespace arguments are provided. However, there is an argparse issue (bpo-18943) where mutual exclusivity is not enforced when --indent=4 is specified (since 4 is default).

For example, --compact --tab or --indent=3 --compact or --indent=4 --compact --tab will all throw errors of the form:

python -m json.tool: error: argument --tab: not allowed with argument --compact

--indent=4 --compact (or --indent=4 --tab) does not throw an error. In the case that --indent=4 is specified with --compact, --tab, or no-indent, the later will take precedent.

Loading

@mya12321
Copy link

@mya12321 mya12321 commented Apr 8, 2018

I hope this feature can be merged ASAP, it's really helpful as a cross-platform command line utility.

Loading

@dhimmel
Copy link
Contributor Author

@dhimmel dhimmel commented Apr 8, 2018

I hope this feature can be merged ASAP

@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.

Loading

@methane
Copy link
Member

@methane methane commented Dec 1, 2019

Would you resolve the conflict?

Loading

@methane methane merged commit 0325794 into python:master Dec 4, 2019
4 checks passed
Loading
sthagen referenced this issue in sthagen/python-cpython Dec 4, 2019
bpo-29636: Add --(no-)indent arguments to json.tool (GH-345)
@vstinner
Copy link
Member

@vstinner vstinner commented Dec 4, 2019

Well done @dhimmel and thanks for your tenacity!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants