Python 3.5 support #81
Conversation
|
+1 |
|
+1 for merge |
|
Is there anything we can do to help with getting this merged? |
|
AS another data point: |
|
Any chance this can get into master? Need it big time. Thanks. |
| if not needs_class_hack: | ||
| # restore FLOAT_REPR | ||
| json.encoder.FLOAT_REPR = original_float_repr | ||
|
|
There was a problem hiding this comment.
Where does all this code come from?
If it was written specifically for django-geojson then each aspect of it deserves extensive tests...
There was a problem hiding this comment.
As explained in the comments the code is copied from the python3.5 source code ( https://github.com/python/cpython/blob/3.5/Lib/json/encoder.py#L203 ) and thing that is actually changed is def floatstr(..., _repr=float_repr, ...) instead of def floatstr(..., _repr=float.__repr__, ...). The rest of the modifications are prepending json.encoder. where needed.
Btw since python3.6 has been released and the encoder code is the same ( https://github.com/python/cpython/blob/3.6/Lib/json/encoder.py#L204 ) needs_class_hack should be (3, 5) <= sys.version_info < (3, 7)
There was a problem hiding this comment.
I did it differently so it works anywhere: c70a7e4
There was a problem hiding this comment.
@Gagaro I didn't do it your way because I copied a chunk of internal code from the original python class and in the unlikely case that this logic is changed in python3.7, in with my solution it would crash (the tests would fail) and somebody would have to manually inspect it, while in yours it will work, but might sometimes create strange results that would be harder to debug.
Anyway, thanks, glad to see it added to the master :)
|
BTW we can't merge pull requests with broken tests :) |
|
Test were broken on master. I just fixed them. Could you rebase? Thanks 👍 . (Actually they are still broken because I added test for python 3.5, the test should be green with your PR). |
|
Merged on master, I'll release as soon as I have the rights on pypi. Thanks 👍 . |
|
django-geojson 2.10.0 is out. |
Closes #80