Skip to content
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

Change Python version checking due conflicts with libraries that patch urllib patching #36

Merged
merged 5 commits into from Jul 23, 2016

Conversation

@MisterRios
Copy link
Contributor

MisterRios commented Jul 15, 2016

Hi there, I found a conflict with the isort library in which urllib is patched: https://github.com/timothycrosley/isort/blob/develop/isort/pie_slice.py#L135
This causes urlobject to use _qs_encode_py2 and _qs_decode_py2 functions.

In Python3, this throws a NameError: name 'unicode' is not defined as Python3 no longer has the unicode type.

Found via a Django project using spurl which has urlobject as a dependency.

@MisterRios

This comment has been minimized.

Copy link
Contributor Author

MisterRios commented Jul 15, 2016

I realized in e8b3b8d that versions after Python2 would not have the unicode type so it's better to check for Python2, and have everything else use the python3 methods- i.e. Theoretically a python4 would still be using the python3 methods.

@jonasvp

This comment has been minimized.

Copy link
Contributor

jonasvp commented Jul 20, 2016

Concur - checking for a stdlib attribute as a way to differentiate between Python versions seems very roundabout. Any way to get this released?

@MisterRios You might want to push this as a single commit.

@agriffis

This comment has been minimized.

Copy link
Collaborator

agriffis commented Jul 20, 2016

Since we're already importing from six, could you change this to use PY2 from there? Otherwise looks good.

@MisterRios

This comment has been minimized.

Copy link
Contributor Author

MisterRios commented Jul 21, 2016

@agriffis The version of six bundled here only checks for PY3- https://github.com/zacharyvoase/urlobject/blob/master/urlobject/six.py#L31

The source six does check for PY2, but it's not available in this repo- https://bitbucket.org/gutworth/six/src/8b024292064c9d2fbd7a4f352c40b261b91fd058/six.py?fileviewer=file-view-default#six.py-36

The reason I check for PY2 in particular is that only python2 will use unicode. As of Python3, and forward, there is no unicode keyword anymore. If there is ever a Python 4, and it only checks for PY3, then it will revert to using the python2 functions.

@MisterRios

This comment has been minimized.

Copy link
Contributor Author

MisterRios commented Jul 21, 2016

@agriffis I've inserted PY2 into the bundled six module, and imported from there. If the bundled six here is removed ever, then an import of PY2 from six would still work.

The possible future change in query_string.py would be:
from .six import PY2 -> `from six import PY2"

@agriffis

This comment has been minimized.

Copy link
Collaborator

agriffis commented Jul 23, 2016

@MisterRios Thanks, this seems like a sensible approach. If we unbundle six eventually then we'll just rely on a sufficiently recent version.

@agriffis agriffis merged commit 0db49a6 into zacharyvoase:master Jul 23, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jonasvp

This comment has been minimized.

Copy link
Contributor

jonasvp commented Jul 26, 2016

@agriffis Since the last release was a while ago - would it be possible to roll a quick point release with this commit? Our CI server (and we as well) would be extremely grateful!

@agriffis

This comment has been minimized.

Copy link
Collaborator

agriffis commented Aug 6, 2016

@jonasvp v2.4.2 now released on PyPI!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.