Refactor/pretty format json #254

cas-- · 2017-12-10T09:38:18Z

Two commits to fix pretty_format_json:

Remove pretty_format_json simplejson dependency

The simplejson module is only needed for <=py25 so replace with builtin json.
Replace six dependecy for simple Py2 check for convertion to unicode.
Cleanup quotes.

Fix pretty_format_json to use int indent

The indent parameter for json should be integer and under Python2 is
will raise an error if not. So switch from str to int and mention
default value in help text.

asottile

Thanks for the PR!

A few little things below, overall I'm + on removing dependence on simplejson

asottile · 2017-12-10T16:53:15Z

setup.py

@@ -28,8 +28,6 @@
        'flake8!=2.5.3',
        'autopep8>=1.3',
        'pyyaml',
-        'simplejson',
-        'six',


six is used elsewhere so it can't be removed

I missed that other usage, that can be easily resolved too. :)

tbh, until we're comfortable dropping python 2 entirely, keeping six around seems ok to me -- I'd certainly rather see from six.moves import ... than try: except: for imports

For a single import to require six, I'd disagree as it creates an extra level of abstraction to a 3rd party module and a docs lookup for usage. With a try..except you know exactly what is going on. Both will have to be modified in future regardless though I suppose it'd then be easier to find the six module imports...

Anyway, it's not a big deal :)

asottile · 2017-12-10T16:53:34Z

pre_commit_hooks/pretty_format_json.py


                if args.autofix:
                    _autofix(json_file, pretty_contents)

                status = 1

-        except simplejson.JSONDecodeError:
+        except json.JSONDecodeError:


there's no json.JSONDecodeError, I believe stdlib throws ValueError?

Ah it does exist on Py3 but not Py2... I'll sort this 👍

asottile · 2017-12-10T16:54:36Z

pre_commit_hooks/pretty_format_json.py

+    # Ensure unicode under Py2
+    if sys.version_info.major == 2:
+        json_pretty = json_pretty.decode()
+    json_pretty += '\n'  # dumps do not end with a newline


the comment was changed here, though I think the lhs has better grammar

Yeah intentional :)

I guess it makes more sense to me that dumps is a singular noun ("dump string")

asottile · 2017-12-10T16:55:35Z

pre_commit_hooks/pretty_format_json.py

-    )) + "\n"  # dumps does not end with a newline
+    )
+    # Ensure unicode under Py2
+    if sys.version_info.major == 2:


will need a no cover comment to satisfy coverage.

Since six is still used, probably just use six.PY2 here

asottile · 2017-12-10T16:58:56Z

pre_commit_hooks/pretty_format_json.py

-        default='  ',
-        help='String used as delimiter for one indentation level',
+        default=2,
+        help='String used as delimiter for one indentation level (Default: 2)',


this shouldn't be refactored -- it allows for the following inputs:

'\t'

' '

'4'

See #120 which is essentially the opposite of this patch :)

Hmm ok, strings break Py2 json. Suggestions? Keep simplejson and have json as fallback?

There is no need for that function tbh since both simplejson and py3 json accept both str and int and do the conversion...

'' and '4' are still value and coverted to 0 and 4 ints.

Ok what I suggest is to leave the default as an int (because that is the original implementation of indent), pass the values str or int to json and catch any exceptions.

>>> '\t'.strip() ''

hmmm seems to do what I expect :)

Oh I see you edited, disregard :D

Yeah I was testing with strip(' ') not strip() 😱

I think I have the solution for all this and will update...

hmmm yes, a bit of a predicament then...

I guess we could just document that \t is invalid for python2 and if that feature is desired use python 3? The tests'll need some adjustment to show this behaviour (and the README will need an update).

cas-- · 2017-12-10T18:16:20Z

btw I thought I had run the tests before creating the PR, sorry about that!

* The simplejson module is only needed for <=py25 so replace with builtin json. * Replace six dependecy for simple Py2 check for convertion to unicode. * Cleanup quotes.

The indent parameter for json should be integer and under Python2 is will raise an error if not. So switch from str to int and mention default value in help text.

cas-- · 2017-12-10T22:34:18Z

This was a fun project... much head scratching with failing tests, found a bug in object_pairs_hook for py2.7 and 3.4: https://bugs.python.org/issue16333

Anyway all sorted, I think...

asottile · 2017-12-11T15:32:32Z

ah yeah, I've hit that json bug before, fun fun!

asottile · 2017-12-11T16:33:18Z

Thanks again!

asottile reviewed Dec 10, 2017

View reviewed changes

cas-- force-pushed the refactor/pretty-format-json branch 2 times, most recently from 6e4d4c6 to e472253 Compare December 10, 2017 22:19

Calum Lind added 2 commits December 10, 2017 22:33

Remove pretty_format_json simplejson dependency

00974ef

* The simplejson module is only needed for <=py25 so replace with builtin json. * Replace six dependecy for simple Py2 check for convertion to unicode. * Cleanup quotes.

Fix pretty_format_json to use int indent

5b6ddaf

The indent parameter for json should be integer and under Python2 is will raise an error if not. So switch from str to int and mention default value in help text.

cas-- force-pushed the refactor/pretty-format-json branch from e472253 to 5b6ddaf Compare December 10, 2017 22:33

Add six back to setup.py

3e1b954

asottile approved these changes Dec 11, 2017

View reviewed changes

asottile merged commit cf04ab0 into pre-commit:master Dec 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/pretty format json #254

Refactor/pretty format json #254

cas-- commented Dec 10, 2017

asottile left a comment

asottile Dec 10, 2017

cas-- Dec 10, 2017

asottile Dec 10, 2017

cas-- Dec 10, 2017 •

edited

Loading

asottile Dec 10, 2017

cas-- Dec 10, 2017

asottile Dec 10, 2017

cas-- Dec 10, 2017

asottile Dec 10, 2017

asottile Dec 10, 2017

asottile Dec 10, 2017

cas-- Dec 10, 2017

cas-- Dec 10, 2017 •

edited

Loading

cas-- Dec 10, 2017 •

edited

Loading

asottile Dec 10, 2017

asottile Dec 10, 2017

cas-- Dec 10, 2017

asottile Dec 10, 2017

cas-- commented Dec 10, 2017

cas-- commented Dec 10, 2017

asottile commented Dec 11, 2017

asottile commented Dec 11, 2017

Refactor/pretty format json #254

Refactor/pretty format json #254

Conversation

cas-- commented Dec 10, 2017

asottile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cas-- Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cas-- Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

cas-- Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cas-- commented Dec 10, 2017

cas-- commented Dec 10, 2017

asottile commented Dec 11, 2017

asottile commented Dec 11, 2017

cas-- Dec 10, 2017 •

edited

Loading

cas-- Dec 10, 2017 •

edited

Loading

cas-- Dec 10, 2017 •

edited

Loading