-
Notifications
You must be signed in to change notification settings - Fork 12
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
Py2 and 3 compatibility #40
Comments
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! |
I believe I tracked down the source of the issue, and it is not the fault of this library. We have been using >>> 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. |
@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, Currently, the strings created by The only solution that I see to this problem is to make sure that all strings created by The issue is actually larger than Python 2 / 3 compatibility since there are instrumentation libraries that call 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. |
I think your evaluation is correct. If this library is going to be compatible with codebases that have called I haven't tested it, but it may be as simple as modifying imgix-python/imgix/urlhelper.py Line 48 in 5d49f63
u -prefix.
|
Also just to clarify, the issue in Python 3 isn't mixing of This is a Python 3 shell with just the standard library, no
|
@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? |
Hi @jnak 👋 actually I am this project's maintainer. I'd be happy to look at a PR if you were to open one. |
@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. |
@sherwinski When are you planning to release this patch? |
At Rover we are upgrading our codebase to python3.6 while maintaining compatibility with python2.7.
We are running into a problem where imgix isn't compatible with both python 2.7 and 3.6 at the same time.
The reason is that the behavior of string literals in the imgix-python library is not consistent between python versions.
For example, take
URLBuilder.create_url(path, opts)
. Ifpath
is explicitly unicode in python 2, this will result in the following error:This will fail in the same way if
path
is explicitly a byte string in python 3. The only way that it can be expected to work at all between the two versions is if thepath
variable is a string literal with no explicit type andfrom __future__ import unicode_literals
is not present.In order to ease our transition of hundreds of thousands of lines of code to python3 we need to include all future imports in all our files which means that
path
will be explicitly unicode in both python2 and python3.The issue can be fixed by making string literals be either explicitly unicode or bytestring regardless of what version of python was being run. This can be done by adding
from __future__ import unicode_literals
to all the files in this library.One way to automatically do this would be to use futurize
The text was updated successfully, but these errors were encountered: