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

Fix encoding bugs in Python2 (non-ASCII characters) #80

Merged
merged 7 commits into from
Sep 19, 2016

Conversation

MiguelSR
Copy link
Collaborator

@MiguelSR MiguelSR commented Sep 8, 2016

Fix bugs in Python2 when column name has non-ascii characters and when Field description has non-ascii characters.

@MiguelSR MiguelSR changed the title Fix bug in Python2 when column name has non-ascii characters. Fix encoding bugs in Python2 (non-ASCII characters) Sep 8, 2016
@graysonarts
Copy link
Contributor

Hi @MiguelSR - Thank you so much for finding and fixing this issue!

I'm going to spend some time today to write a test case that exercises this for py2 and py3. In order to be able to accept the PR, we do need you to sign our contributor license agreement which is available on tableau.github.io and can be signed electronically by emailing the address listed there.

Also, do you mind rebasing this PR against the development branch? We keep master equal to our latest released version, and we only release a version every month, so this would need to live in development until we release the next version at the end of September.

if sys.version_info[0] == 2:
description_string = description_string.decode('utf-8')

return u'{}'.format(description_string) # This is necessary for py3 support
Copy link
Contributor

@t8y8 t8y8 Sep 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be done with a typecheck instead?

if isinstance(description_string, bytes):
    description_string = description_string.decode('utf-8')
return description_string

Then I believe the u'{}' format dance isn't necessary

(And we don't have to import sys and do the version check)

If we must do a version check, can you put it at the top

PY2 = sys.version_info[0] == 2
...
if PY2:
    <format dance>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello.

Yes it makes sense to check the type instead of the Python version.

I'll sign later the CLA, do the rebase and fix the commit to get rid of sys.

Thanks,
Miguel.

@t8y8
Copy link
Contributor

t8y8 commented Sep 15, 2016

@MiguelSR -- Wanted to ping you and see if you were still interested in submitting a patch?

(We love having more contributors, let us know if you need anything)

@MiguelSR
Copy link
Collaborator Author

Yes sure, I was waiting for the cla, which was sent to me today :)

El 15 sept. 2016 22:26, "Tyler Doyle" [email protected] escribió:

@MiguelSR https://github.com/MiguelSR -- Wanted to ping you and see if
you were still interested in submitting a patch?

(We love having more contributors, let us know if you need anything)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#80 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEn9aFbgPYQHVIiKztjNEsycL_06MGuhks5qqanxgaJpZM4J34lv
.

@benlower
Copy link
Contributor

@MiguelSR thanks for your patience while we got the CLA sent out. You are good to go now.
@t8y8 @RussTheAerialist Miguel signed the CLA so we can accept his contributions now.

@t8y8 t8y8 added bug and removed CLARequired labels Sep 18, 2016
@MiguelSR MiguelSR changed the base branch from master to development September 19, 2016 08:08
@graysonarts graysonarts merged commit b80333d into tableau:development Sep 19, 2016
@graysonarts
Copy link
Contributor

Thank you so much @MiguelSR!

@MiguelSR
Copy link
Collaborator Author

Thank you :)

El 19 sept. 2016 17:59, "Russell Hay" [email protected] escribió:

Thank you so much @MiguelSR https://github.com/MiguelSR!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#80 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEn9aA4QePVB54t_4lFV671VedOwiYDzks5qrrF2gaJpZM4J34lv
.

graysonarts pushed a commit that referenced this pull request Sep 19, 2016
graysonarts pushed a commit that referenced this pull request Oct 7, 2016
* Updating link in readme

We renamed Examples to samples, but forgot to update the readme.

* Adding #80 to changelog
graysonarts pushed a commit that referenced this pull request Oct 7, 2016
graysonarts pushed a commit that referenced this pull request Oct 7, 2016
* Updating link in readme

We renamed Examples to samples, but forgot to update the readme.

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

Successfully merging this pull request may close these issues.

5 participants