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

isinstance(filename, "utf-8") #304

Closed
mnowotka opened this issue Jul 25, 2013 · 10 comments

Comments

Projects
None yet
4 participants
@aclark4life

This comment has been minimized.

Copy link
Member

commented Jul 25, 2013

Not sure what you are asking for but you are welcome to send a pull request

@mnowotka

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2013

Easy - according to content of the error, the second argument of isinstance can't be string (i.e. 'utf-8'). 'isinstance' is not the way how you check file encoding. I'm not sure what is a propper way (that's why I asked it on SO) but the current implementation is clearly wrong.

@aclark4life

This comment has been minimized.

Copy link
Member

commented Jul 25, 2013

How about isinstance(filename, unicode) ? (HT: @mjpieters)

@mjpieters

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2013

isinstance(filename, str) should work. It is not isinstance() and the code is trying to convert it to str because that is the right type for both Python 2 and 3 here.

@mnowotka

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2013

https://enjoydoingitwrong.wordpress.com/2009/06/22/unicode-is-not-utf/
so I'm not sure if you want to check encoding (in that case isinstance(filename, unicode) is not the answer) or just check if the content is instance of basestring or unicode.

@mnowotka

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2013

@mjpieters - if you know it for sure, can you change it?

@aclark4life

This comment has been minimized.

Copy link
Member

commented Jul 25, 2013

@mnowotka You can change it by sending a pull request 😃

mjpieters added a commit to mjpieters/Pillow that referenced this issue Jul 25, 2013

Fix for python-pillow#304: test for `str`, not `"utf-8"`.
The code wants to produce a `str` object for the given Python version (which is the right thing to do here).
@wiredfool

This comment has been minimized.

Copy link
Member

commented Jul 25, 2013

That (original) code looks like a double encoding bug waiting to happen.

@mjpieters

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2013

@wiredfool: No, it's fine; Python 2 not str means unicode, so it needs to be encoded, in Python 3, not str means bytes, so it needs to be decoded.

aclark4life added a commit that referenced this issue Jul 25, 2013

Merge pull request #306 from mjpieters/master
Fix for #304: test for `str`, not `"utf-8"`.
@wiredfool

This comment has been minimized.

Copy link
Member

commented Jul 25, 2013

@mjpieters fair enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.