Skip to content

bpo-29636: improve CLI of json.tool #9765

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

Closed
wants to merge 2 commits into from
Closed

bpo-29636: improve CLI of json.tool #9765

wants to merge 2 commits into from

Conversation

wimglenn
Copy link
Contributor

@wimglenn wimglenn commented Oct 9, 2018

Added ability to pass through indent and ensure_ascii options in json.tool command-line interface.

Related issues are 27413 and 29636.

There is other PR existing: #345 - that one is stagnating because @serhiy-storchaka commented on the bpo thinking it should not be merged. Serhiy claimed it adds too many (potentially conflicting) options, and I would tend to agree. This alternate implementation is a more straight-forward pass-thru of the relevant options to json.dumps.

Some motivation: How to use json.tool from the shell to validate and pretty-print language files without removing the unicode?

https://bugs.python.org/issue29636

parser.add_argument('--sort-keys', action='store_true', default=False,
help='sort the output of dictionaries alphabetically by key')
options = parser.parse_args()

infile = options.infile or sys.stdin
outfile = options.outfile or sys.stdout
sort_keys = options.sort_keys
try:
indent = int(options.indent)
except ValueError:
Copy link
Contributor

Choose a reason for hiding this comment

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

if you use type in argparse you can remove this section

Copy link
Contributor Author

@wimglenn wimglenn Oct 11, 2018

Choose a reason for hiding this comment

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

No, then it will be impossible to pass tab character through to json dumps from the command-line, for example by using --indent=" ".

Copy link
Contributor

Choose a reason for hiding this comment

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

ack!

@dhimmel
Copy link
Contributor

dhimmel commented Oct 15, 2018

Author of bpo-29636 and the associated PR #345 to add indentation options to json.tool here. I also have an open PR (meant to come after the --indent changes) in #201 to add --no-ensure-ascii. Happy to see interest from the participants here to get these features added, but I've been maintaining these open PRs for over a year now and they have undergone considerable review and discussion. Hence, I'm not sure the right way forward is for new PRs to propose the same functionality... rather than say getting involved in the BPO discussion such that consensus can be reached that these are valuable features to add.

There is other PR existing: #345 - that one is stagnating because @serhiy-storchaka commented on the bpo thinking it should not be merged. Serhiy claimed it adds too many (potentially conflicting) options, and I would tend to agree. This alternate implementation is a more straight-forward pass-thru of the relevant options to json.dumps.

I initially started out my PR with only supporting --indent very similar to what is proposed here. However, discussion at #201 (comment), #2720 (comment), and #345 (comment) pushed my PR to support more indentation options along the lines of the jq utility. However, as I note in the BPO discussion, I'm happy to remove any of the mutually exclusive indentation options at the maintainers / community's discretion. I just thought it'd be easiest to work backwards.

Anyways, @wimglenn happy to have you involved, if you'd like to help with any of the open PRs and discussion on BPO.

@dhimmel
Copy link
Contributor

dhimmel commented Dec 4, 2019

@wimglenn #345 was merged into 0325794 providing indentation control for json.tool.

I also have #201 open providing an option for ensure_ascii. I'd be happy to close that PR in favor of this PR, if you are interesting in updating the scope to just be the ascii control and maintaining the PR until its merged.

@wimglenn
Copy link
Contributor Author

wimglenn commented Dec 5, 2019

Hey Daniel, thanks for the ping. That's a little bizarre, the last discussion on issue29636 was

@serhiy-storchaka

I don't think this PR should be merged. It adds too much options.

@rhettinger

I'm inclined to agree with Serhiy.

Then suddenly 4 days ago @methane asks "would you resolve the conflict?" and it's merged without further ado? What changed? Did these two core developers change their mind in some offline discussion? Why was it merged with no documentation, there should be a change in Doc/library/json.rst "Command line options" section?

I mean I'm glad to have the feature in, even if I think the #345 was a little too complicated it's better than not having the functionality at all. I don't have much interest in reworking this PR for the --ensure-ascii, it's still labelled "awaiting core review" with not a peep from core in more than a year so I think I'll just be wasting my time and effort.

@wimglenn wimglenn closed this Dec 5, 2019
@methane
Copy link
Member

methane commented Dec 5, 2019

I merged #345 because I want to reduce open PRs and issues. I thought about I should reject it or merge it.
I decided to merge it because rejecting it doesn't make sense after we merged --json-lines option.

json-lines is not valid JSON, and output of the option is not a valid json-lines too.
I don't think the number of users of --indent option is significantly smaller than --json-lines.

@wimglenn
Copy link
Contributor Author

wimglenn commented Dec 5, 2019

@methane "To reduce open PRs and issues" is a noble goal, but you don't request documentation for the change to go in alongside the change itself? And to disregard/ignore the comments from other core developers? --json-lines is entirely orthogonal to their concerns raised.

I'll put the --no-ensure-ascii part to a new PR, since it's issue27413 not 29636.

@methane
Copy link
Member

methane commented Dec 5, 2019

but you don't request documentation for the change to go in alongside the change itself?

Yes, I noticed I missed it. It should be added.

And to disregard/ignore the comments from other core developers?

Sometime, it is difficult to make a final agreement.

We discuss in python-dev mailing list If the problem is important enough.
But in this case, I didn't think this issue is important enough. So I merged it by my own judgment.

I think this is important to reduce open PRs and issues. We can not discuss and make a decision in the mailing list for all feature requests. Time is a limited resource.

Of course, I might be wrong and other core devs oppose it strongly. We can revert it then.

@wimglenn
Copy link
Contributor Author

wimglenn commented Dec 5, 2019

OK. Well, it is good that we are burning down open PRs and issues at least. Poor @dhimmel was trying to move this since Feb 2017 (!)

Here is the other part (created a new PR because this one has wrong bpo number now) --> #17472

Off-topic: that is a really cute dog @methane ... is a long haired dachshund?

@dhimmel
Copy link
Contributor

dhimmel commented Dec 5, 2019

Why was it merged with no documentation, there should be a change in Doc/library/json.rst "Command line options" section?

I am happy to make another PR to add this documentation for #345. PR with documentation now up at #17482.

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

Successfully merging this pull request may close these issues.

6 participants