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

dhimmel · 2017-07-15T19:00:47Z

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:

bpo-29636: Add --indent / --no-indent arguments to json.tool #345 to add indentation / whitespace options (bpo-29636).
bpo-27413: add --no-ensure-ascii argument to json.tool #201 to display non-ascii characters (bpo-27413).

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 · 2017-07-15T19:00:49Z

@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

I left inline comments for the motivation behind each change.

dhimmel · 2017-07-15T19:06:57Z

Lib/json/tool.py

@@ -25,22 +25,22 @@ def main():
                        help='a JSON file to be validated or pretty-printed')
    parser.add_argument('outfile', nargs='?', type=argparse.FileType('w'),
                        help='write the output of infile to outfile')
-    parser.add_argument('--sort-keys', action='store_true', default=False,
-                        help='sort the output of dictionaries alphabetically by key')


This line exceeded the PEP8 line length, hence the rephrasing.

dhimmel · 2017-07-15T19:07:19Z

Lib/json/tool.py

        except ValueError as e:
            raise SystemExit(e)
+
+    # Output JSON
+    outfile = options.outfile or sys.stdout


Move variable definition close to where it's used.

dhimmel · 2017-07-15T19:07:48Z

Lib/json/tool.py

    sort_keys = options.sort_keys
+    hook = collections.OrderedDict if sort_keys else None


Remove the heavily indented if-else for readability

dhimmel · 2017-07-15T19:08:10Z

Lib/json/tool.py

@@ -25,22 +25,22 @@ def main():
                        help='a JSON file to be validated or pretty-printed')
    parser.add_argument('outfile', nargs='?', type=argparse.FileType('w'),
                        help='write the output of infile to outfile')
-    parser.add_argument('--sort-keys', action='store_true', default=False,


Remove unnecessary argument.

dhimmel · 2017-07-15T19:08:34Z

Lib/json/tool.py

    options = parser.parse_args()

+    # Read input JSON


Comment for readability.

bitdancer · 2017-07-15T20:38:37Z

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

nirs · 2017-07-17T00:07:38Z

Lib/json/tool.py

        except ValueError as e:
            raise SystemExit(e)
+
+    # Output JSON
+    outfile = options.outfile or sys.stdout


Not sure this change helps, it is easier to read when we process all options in one place, and then use the process options.

It would be nice if we could integrate this in options:

outfile = options.outfile or sys.stdout

Does this work?

parser.add_argument('outfile', nargs='?', type=argparse.FileType('w'), default=sys.stdout, help='write the output of infile to outfile')

nirs · 2017-07-17T00:08:13Z

Lib/json/tool.py

-    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 dictionary outputs alphabetically by key')


... dictionaries output ...?

@nirs I changed it to:

sort dictionaries output alphabetically by key

as per your suggestion. But would it make more sense as possessive:

sort dictionaries' output alphabetically by key

I would check other modules and use the same style.

I would phrase this as "sort alphabetically by key when generating dictionary output".

nirs

Nice!

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 · 2017-07-18T16:01:01Z

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 · 2017-07-18T18:58:16Z

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 · 2017-07-19T19:54:51Z

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

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

Refs https://bugs.python.org/msg298692. Code was descriptive without the comments.

dhimmel · 2017-07-20T15:35:45Z

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.

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.

json.tool: improve code readability

5639cb3

the-knights-who-say-ni added the CLA signed label Jul 15, 2017

dhimmel commented Jul 15, 2017

View reviewed changes

nirs reviewed Jul 17, 2017

View reviewed changes

Address python#2720 (review)

ff3b029

nirs approved these changes Jul 17, 2017

View reviewed changes

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 changed the title ~~Improve code readability of json.tool~~ bpo-30971: Improve code readability of json.tool Jul 19, 2017

Remove comments

143d63b

Refs https://bugs.python.org/msg298692. Code was descriptive without the comments.

dhimmel closed this Jul 20, 2017

dhimmel mentioned this pull request Feb 22, 2018

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

Merged

dhimmel mentioned this pull request Oct 15, 2018

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

Closed

		sort_keys = options.sort_keys
		hook = collections.OrderedDict if sort_keys else None

Uh oh!

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

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

Uh oh!

Conversation

dhimmel commented Jul 15, 2017

Uh oh!

mention-bot commented Jul 15, 2017

Uh oh!

dhimmel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitdancer commented Jul 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirs left a comment

Choose a reason for hiding this comment

Uh oh!

dhimmel commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bitdancer commented Jul 18, 2017

Uh oh!

dhimmel commented Jul 19, 2017

Uh oh!

dhimmel commented Jul 20, 2017

Uh oh!

Uh oh!

dhimmel commented Jul 18, 2017 •

edited

Loading