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

Unicode handling #135

Closed
brutasse opened this issue Apr 29, 2016 · 6 comments · Fixed by #138
Closed

Unicode handling #135

brutasse opened this issue Apr 29, 2016 · 6 comments · Fixed by #138

Comments

@brutasse
Copy link
Contributor

In various places py-trello .encode('utf-8') some strings returned by the API.

What's the rationale behind this? Trello data is returned in JSON, with string keys and attributes in unicode strings (u'foo' in Python 2, 'foo' in Python 3).

Keeping unicode would make sense: in Python 2 you can compare bytestrings to unicode strings implicitly (u'foo' == 'foo'), but in Python 3 it'd make more sense to compare e.g. board.name to unicode strings instead of bytestrings. In Python 3 our code needs to re-decode data for comparison or string formatting. We have things like:

if board.name == b'Board name'

or:

message = '{}: {}'.format(board.name.decode(), card.name.decode())

My suggested fix would be to remove all occurrences of .encode('utf-8') here, to have a proper unicode sandwich behavior. Any objections? I'm happy to submit a PR.

@brutasse
Copy link
Contributor Author

Refs #66 #118

@sfriesel
Copy link

sfriesel commented May 3, 2016

this is the first commit that introduced the encoding: c925101 It was triggered by a wrong implementation of repr in the previous commit 2914118

@sarumont
Copy link
Owner

sarumont commented Jun 1, 2016

No objections to proper UTF-8 fixes. I merged the previous PR, as I didn't have the time to properly review it. :) PRs are welcome!

@sfriesel
Copy link

sfriesel commented Jun 6, 2016

choose your poison :)

@sarumont
Copy link
Owner

Thanks for the work, guys! I merged #138, and I bumped the version to 0.6.0. Anyone who is depending on the old, broken behavior can still use 0.5.1. #139 appears to have been a subset of #138, so I won't worry about that unless I missed something @sfriesel.

@brutasse
Copy link
Contributor Author

Thanks for the merge, @sarumont!

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

Successfully merging a pull request may close this issue.

3 participants