◐ Shell
reader mode source ↗
Skip to content

bpo-30971: Improve code readability of json.tool#2720

Closed
dhimmel wants to merge 4 commits into
python:masterfrom
dhimmel:json-tool-refactor
Closed

bpo-30971: Improve code readability of json.tool#2720
dhimmel wants to merge 4 commits into
python:masterfrom
dhimmel:json-tool-refactor

Conversation

@dhimmel

@dhimmel dhimmel commented Jul 15, 2017

Copy link
Copy Markdown
Contributor

Several months ago I worked on pull requests to add new options to the json.tool command line utility. These pull requests are still open, pending further design discussion and consensus on whether to add them:

Related, #346 fixed a small testing bug in json.tool and was merged.

Since #345 and #201 have been inactive for some time, I wanted to take the non-controversial code changes from those PRs. This PR contains readability improvements and minor refactoring to json.tool. The goal is to get some of the previous work merged and make the json.tool source easier to maintain and enhance going forward. Admittedly, the changes are quite minor.

@mention-bot

mention-bot commented Jul 15, 2017

Copy link
Copy Markdown

@dhimmel, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @tiran and @berkerpeksag to be potential reviewers.

dhimmel
dhimmel commented Jul 15, 2017
View reviewed changes

@dhimmel dhimmel left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hide comment

I left inline comments for the motivation behind each change.

@bitdancer

bitdancer commented Jul 15, 2017

Copy link
Copy Markdown
Member

We generally don't make changes like these unless we are modifying the code for other reasons.

nirs
nirs approved these changes Jul 17, 2017
View reviewed changes

@nirs nirs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Nice!

@dhimmel
Update --sort-keys help message …
61cbd23 
Use --sort-keys help message based on the json.tool docs at
https://docs.python.org/3.6/library/json.html#basic-usage:

> If sort_keys is true, then the output of dictionaries
> will be sorted by key.
@dhimmel

dhimmel commented Jul 18, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

If this issue is so simple that there’s no need for an issue to track any discussion of what the pull request is trying to solve (e.g. fixing a spelling mistake), then the pull request needs to have the “skip issue” label added to it.

bedevere/issue-number is failing. I assumed this PR doesn't require an issue, but let me know if otherwise.

@bitdancer

bitdancer commented Jul 18, 2017

Copy link
Copy Markdown
Member

This absolutely needs an issue to discuss whether or not to apply it. We probably won't, as we don't normally accept "prettify" pull requests.

@dhimmel dhimmel changed the title Improve code readability of json.tool Jul 19, 2017
@dhimmel

dhimmel commented Jul 19, 2017

Copy link
Copy Markdown
Contributor Author

This absolutely needs an issue to discuss whether or not to apply it.

Thanks @bitdancer for the guidance. I opened bpo-30971.

@dhimmel
Remove comments …
143d63b 
Refs https://bugs.python.org/msg298692.
Code was descriptive without the comments.
@dhimmel

dhimmel commented Jul 20, 2017

Copy link
Copy Markdown
Contributor Author

I'm closing the pull request in response to the advice received in bpo-30971.

Specifically from @bitdancer:

I think using this PR as a base for your feature PRs, and then committing everything together if they are accepted, would be the way to go.

And from @berkerpeksag:

Everyone has their own preferences and it's usually not enough to justify code churn.

Will rebase my other PRs with json.tool enhancements on top of these changes.

@dhimmel dhimmel closed this Jul 20, 2017
@dhimmel dhimmel mentioned this pull request Feb 22, 2018
Merged
@dhimmel dhimmel mentioned this pull request Oct 15, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bitdancer bitdancer bitdancer left review comments

+1 more reviewer

@nirs nirs nirs approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dhimmel @mention-bot @bitdancer @nirs @the-knights-who-say-ni