Python 2.7 compatibility (other than type hints) - Githubissues

OCHA-DAP / hdx-python-api

Python API for interacting with the HDX Data Portal

http://data.humdata.org

MIT License

80 stars 16 forks source link

Python 2.7 compatibility (other than type hints) #2

Closed bdon closed 7 years ago

bdon commented 7 years ago

Without type hints, 3 test failures on python2.7 (still investigating)

This PR is fixing module name clashes and using six to resolve differences in syntax and the standard library

coveralls commented 7 years ago

Coverage increased (+0.02%) to 95.663% when pulling e80e0c0340ee89c17fe0e617736b1e01062488c3 on bdon-python27 into b5c8f21ea001fa0673f201d9033694555467d7a2 on master.

coveralls commented 7 years ago

Coverage increased (+0.05%) to 95.693% when pulling 5fa0ea82a27d865045f8f25649f816f3c36421bd on bdon-python27 into b5c8f21ea001fa0673f201d9033694555467d7a2 on master.

bdon commented 7 years ago

OK, all tests passing on python 2.7. The workaround is a bit hacky in smoothing over the difference between 3 collections.UserDict and 2.7 UserDict.UserDict, since the code iterates over the dict using for we need a 2.7 UserDict.IterableUserDict, which six doesn't handle.

Also 3's UserDict __init__ seems to call __setitem__ for each element in the initial_data parameter, but 2's IterableUserDict doesn't, so this needs to be done by hand, at least for the behavior in the class that depends on it (Dataset). But behavior outside of what's covered by the test suite may be different

mcarans commented 7 years ago

Thanks @bdon I will review. If I merge in your pull request and then @tim6her makes a pull request with type hints, is this likely to cause merge conflicts? I am not sure how well Git's merging will handle it. If you have suggestions about it, I'd be glad to hear them. If not or you don't know, I guess I'll find out by trying!

tim6her commented 7 years ago

Dear @mcarans , my studies are demanding all of my time this week so I won't finish the type hints before Friday. If you complete the review by then, I will just fetch @bdon 's changes and continue the reformatting from there.

mcarans commented 7 years ago

@tim6her Great, I will endeavour to complete by then.

mcarans commented 7 years ago

@bdon I see that the changes you have made allow the Python 3 tests to pass which is great. I am thinking that @tim6her should work off your branch before merging into master because it is not possible to test this PR under Python 2.7 with the Python 3 type hints still in there. Does this make sense?

bdon commented 7 years ago

Makes sense. If you do want to test it on Python 2.7 now, https://github.com/bdon/hdx-python-api/tree/bdon-python27 with HEAD at 1646e63 is the same, with another commit on top that has all type annotations removed.

mcarans commented 7 years ago

@bdon Did you comment the type annotations by hand or is there a tool which does that?

It would be great if there was a tool that converted between Python 3 and Python 2.7 straddling code type annotations and back automatically :-)

bdon commented 7 years ago

Yes, I did it by hand. I don't have any tools to read the type annotations, so can't verify if any changes to them would be correct. Maybe PyCharm has a way to switch to the comment-based annotation mode?

mcarans commented 7 years ago

I have made a request for PyCharm to add that feature but don't expect it any time soon https://youtrack.jetbrains.com/issue/PY-23236

mcarans commented 7 years ago

"If you do want to test it on Python 2.7 now, https://github.com/bdon/hdx-python-api/tree/bdon-python27 with HEAD at 1646e63 is the same, with another commit on top that has all type annotations removed."

The tests there ran fine on 2.7 @tim6her