FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix encoding bugs in Python2 (non-ASCII characters) by MiguelSR · Pull Request #80 · tableau/document-api-python · GitHub

/ document-api-python Public

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

Merged
graysonarts merged 7 commits into
tableau:developmentfrom
MiguelSR:master
Sep 19, 2016
ConversationCommits (7)ChecksFiles changed
Merged

Fix encoding bugs in Python2 (non-ASCII characters)#80
graysonarts merged 7 commits into
tableau:developmentfrom
MiguelSR:master

Conversation

MiguelSR commented Sep 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

MiguelSR mentioned this pull request Sep 8, 2016
Closed
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 commented Sep 8, 2016

Copy link
Copy Markdown
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.

t8y8 reviewed Sep 8, 2016
View reviewed changes
Comment thread tableaudocumentapi/field.py Outdated
if sys.version_info[0] == 2:
description_string = description_string.decode('utf-8')

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

t8y8 Sep 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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>

MiguelSR Sep 9, 2016

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Sep 15, 2016

Copy link
Copy Markdown
Contributor

@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 commented Sep 15, 2016

Copy link
Copy Markdown
Collaborator Author

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

El 15 sept. 2016 22:26, "Tyler Doyle" notifications@github.com 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 commented Sep 17, 2016

Copy link
Copy Markdown
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 added bug and removed CLARequired labels Sep 18, 2016
MiguelSR changed the base branch from master to development September 19, 2016 08:08
graysonarts merged commit b80333d into tableau:development Sep 19, 2016

graysonarts commented Sep 19, 2016

Copy link
Copy Markdown
Contributor

Thank you so much @MiguelSR!

MiguelSR commented Sep 19, 2016

Copy link
Copy Markdown
Collaborator Author

Thank you :)

El 19 sept. 2016 17:59, "Russell Hay" notifications@github.com 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
Add a test to verify #80 (#83)
01f232f
t8y8 mentioned this pull request Oct 7, 2016
Merged
graysonarts pushed a commit that referenced this pull request Oct 7, 2016
Add #80 to changelog (#92)
4ce9cfa 
* 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
Fix encoding bugs in Python2 (non-ASCII characters) (#80)
ee380ac
graysonarts pushed a commit that referenced this pull request Oct 7, 2016
Add a test to verify #80 (#83)
86bb63b
graysonarts pushed a commit that referenced this pull request Oct 7, 2016
Add #80 to changelog (#92)
713eb52 
* Updating link in readme

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

* Adding #80 to changelog
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

graysonarts

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL