-
Notifications
You must be signed in to change notification settings - Fork 17
Python 2.7 compatibility (other than type hints) #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* import UserDict, range, urlparse from six.moves * Configuration and HDXObject inherit from object for python 2.7 * check for unicode string type in addition to str * add six to requirements.txt * use six.raise_from(a,b) instead of raise a from b
* Check for either FileNotFoundError (python 3) or IOError (python 2.7).
* rename logging to hdx_logging and scraperwiki to hdx_scraperwiki
|
|
||
| all_datasets = list() | ||
| dataset = Dataset() | ||
| total_rows = kwargs.get('rows', sys.maxsize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skeptical about this line, do you really want a generator going all the way up to sys.maxsize?
* HDXObject and Configuration either UserDict.IterableUserDict (2.7) or collections.UserDict (3) * on 2.7, call __setitem__ in initializer of IterableUserDict subclass
|
OK, all tests passing on python 2.7. The workaround is a bit hacky in smoothing over the difference between 3 Also 3's |
| super(Dataset,self).__init__({}) | ||
| # workaround: python2 IterableUserDict does not call __setitem__ in __init__, | ||
| # while python3 collections.UserDict does | ||
| for d in initial_data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit hacky, open to suggestions
|
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 Great, I will endeavour to complete by then. |
|
@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? |
|
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. |
|
@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 :-) |
|
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? |
|
I have made a request for PyCharm to add that feature but don't expect it any time soon |
|
"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 |
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