Skip to content
This repository was archived by the owner on Jul 10, 2021. It is now read-only.

Fix unicode messages#47

Open
Ameriks wants to merge 2 commits intokfdm:masterfrom
Ameriks:master
Open

Fix unicode messages#47
Ameriks wants to merge 2 commits intokfdm:masterfrom
Ameriks:master

Conversation

@Ameriks
Copy link

@Ameriks Ameriks commented Jul 22, 2013

There was problem with unicode messages. That is fixed by verifying that incoming variable is basestring (not str) and in case it is not, then we convert variable to unicode (not to str).

There was problem with unicode messages. That is fixed by verifying that incoming variable is basestring (not str) and in case it is not, then we convert variable to unicode (not to str).
@kfdm
Copy link
Owner

kfdm commented Jul 22, 2013

Thanks for the catch. Which version of Python were you using ? Did you run the unit tests through Tox to test all the versions? I'll try to run through the tests myself sometime this week.

@Ameriks
Copy link
Author

Ameriks commented Jul 22, 2013

Sorry, I didn't run tests on all versions, but basestring was first introduced in version 2.3, but it is no longer available in Python 3.

I will update code to pass in all versions of python you have defined in Tox.

@kfdm
Copy link
Owner

kfdm commented Jul 22, 2013

I know the Six library does a few things to check basestring vs str

https://bitbucket.org/gutworth/six/src/e3da7fd96039a6ed89493f89d121c4f3797e6713/six.py?at=default#cl-35

Not sure how we would want to do this here without poking at it some myself. I wonder why it wasn't caught by my existing unicode test

https://github.com/kfdm/gntp/blob/master/gntp/test/basic_tests.py#L47

Do you have an example message that failed ?

@Ameriks
Copy link
Author

Ameriks commented Jul 22, 2013

I'm pretty sure that this will fail, but I was using different text: u"Šaursliežu dzelzceļš"

@Ameriks
Copy link
Author

Ameriks commented Jul 22, 2013

BTW, I didn't use six library, but instead try except. I didn't test it with Tox as I don't have it installed.

Let me know if there is anything else I can help.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants