Py2 and 3 compatibility about imgix-python HOT 10 CLOSED

I think your evaluation is correct. If this library is going to be compatible with codebases that have called install_aliases() from future, then all of the strings passed to urlunparse need to be unicode (or non-unicode).

I haven't tested it, but it may be as simple as modifying

from imgix-python.

As far as I understand, the issue it that in Python 3, parse.urlunsplit raises an error if the params mix both unicode and string (see PythonCharmers/python-future#273 (comment)). That means that codebases that target both Python 2 and Python 3 need to comply to that stricter behavior.

Also just to clarify, the issue in Python 3 isn't mixing of unicode and string (since those are the same type). It's mixing of bytes and str

This is a Python 3 shell with just the standard library, no future / install_aliases():

>>> from urllib.parse import urlunparse
>>> urlunparse([u'https', 'google.com', 'path', '', '', ''])
'https://google.com/path'
>>> urlunparse([b'https', 'google.com', 'path', '', '', ''])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/urllib/parse.py", line 456, in urlunparse
    _coerce_args(*components))
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/urllib/parse.py", line 120, in _coerce_args
    raise TypeError("Cannot mix str and non-str arguments")
TypeError: Cannot mix str and non-str arguments

from imgix-python.

Hi @jnak 👋 actually I am this project's maintainer. I'd be happy to look at a PR if you were to open one.

from imgix-python.

@sherwinski great! I just sent a PR that fixes the issues and adds tests to prevent future regressions. I also verified that my branch fixes the original bug we found. Let me know if you have any questions.

from imgix-python.

Hey @jnak, this patch is now available in v3.1.2 🎉
Thanks again for your help!

from imgix-python.

Hey @nh2d thanks for the ticket and sorry to hear that inconsistent string behavior is causing issues. If I am understanding correctly, it sounds like including all future imports is causing strings to be explicitly typed as unicode, which is causing the error. In trying to understand this issue I attempted to replicate the problem you mentioned without success. The steps I followed, per your description, were:

Python 2.7.10
>>> from __future__ import unicode_literals
>>> import imgix
>>> domain = 'sherwinski.imgix.net'
>>> type(domain)
<type 'unicode'>
>>> imgix.UrlBuilder(domain).create_url('image.jpg')
'https://sherwinski.imgix.net/image.jpg?ixlib=python-2.0.0'

Perhaps it would help with my understanding if you could provide a more concrete example of how to replicate this. Also, I'd be happy to review a PR if you have the time and want to take a swing at the solution for this. Thanks!

from imgix-python.

I believe I tracked down the source of the issue, and it is not the fault of this library.

We have been using futurize to automatically convert our code, and one of the things it tries to do is monkey-patch the standard library with a call to install_aliases when using certain modules (e.g. urllib). With this patch activated, I can reproduce this error:

>>> from future.standard_library import install_aliases
>>> install_aliases()
>>> from imgix import UrlBuilder
>>> UrlBuilder('anything').create_url('anything else')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/dist-packages/imgix/urlbuilder.py", line 112, in create_url
    return str(url_obj)
  File "/usr/local/lib/python2.7/dist-packages/imgix/urlhelper.py", line 188, in __str__
    "", ])
  File "/usr/local/lib/python2.7/dist-packages/future/backports/urllib/parse.py", line 387, in urlunparse
    _coerce_args(*components))
  File "/usr/local/lib/python2.7/dist-packages/future/backports/urllib/parse.py", line 115, in _coerce_args
    raise TypeError("Cannot mix str and non-str arguments")
TypeError: Cannot mix str and non-str arguments

I don't think any change needs to be made to this library.

from imgix-python.

@ascandella Can you elaborate on how you think we should solve this issue in codebases that need to support both Python 2 and Python3?

As far as I understand, the issue it that in Python 3, parse.urlunsplit raises an error if the params mix both unicode and string (see PythonCharmers/python-future#273 (comment)). That means that codebases that target both Python 2 and Python 3 need to comply to that stricter behavior.

Currently, the strings created by imgix when running in Python 2/3 compat mode (ie using future.standard_library.install_aliases) are a mix of unicode and regular strings. For example, self._scheme is a regular strings while query is a unicode (because the patched urllib.quote returns unicode).

The only solution that I see to this problem is to make sure that all strings created by imgix are unicode by annotating them with a u... prefix (see explanation). This can be simplified with from future import unicode_literals but there may be drawbacks That said I don't anticipate issues in imgix because it only manipulates URLs, which are usually treated as unicode anyway. This is the route that the Django project has followed to support both Python 2 and 3.

The issue is actually larger than Python 2 / 3 compatibility since there are instrumentation libraries that call install_aliases under the hood such as opentracing_instrumenation.

Would love to hear your thoughts on that. If you are onboard with this change, I'm happy to send a PR since this is currently blocking us.

from imgix-python.

@nh2d Can we re-open this issue? There are a simple things we can do to allow compatibility with Py2 and Py3. Would you consider a PR if I sent one?

from imgix-python.

@sherwinski When are you planning to release this patch?

from imgix-python.