GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix encoding bugs in Python2 (non-ASCII characters) by MiguelSR · Pull Request #80 · tableau/document-api-python

Conversation

MiguelSR commented Sep 8, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Russell Hay and others added 6 commits

June 29, 2016 09:56


          Merge pull request tableau#39 from RussTheAerialist/master

e6a0bba

Updating README to include information for installing from pip


          Merge pull request tableau#66 from tableau/development

59ac21c

Releasing 0.2 to master


          Update README.md (tableau#71)

bcd73c1

CLANotRequired


          Merge pull request tableau#78 from tableau/development

9a098a7

Release 0.3


          Fix bug in Python2 when column name has non-ascii characters.

3fc9628


          Fix get description method for Python 2 when description has non-ASCI…

473d613

…I characters

MiguelSR mentioned this pull request

Sep 8, 2016

"UnicodeDecodeError" when description has non-ascii characters #81

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

benlower added the CLARequired label

Sep 8, 2016

benlower assigned graysonarts

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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>

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

MiguelSR Sep 9, 2016

Copy link

Copy Markdown

Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

t8y8 reacted with thumbs up emoji

All reactions

👍 1 reaction

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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" [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
.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

t8y8 added bug and removed CLARequired labels

Sep 18, 2016


          Small refactor, check type of string instead of python version

8de7409

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MiguelSR commented Sep 19, 2016

Copy link

Copy Markdown

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
.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bump version and create changelog #91

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

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

graysonarts

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

MiguelSR commented Sep 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graysonarts commented Sep 8, 2016

Uh oh!

t8y8 Sep 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiguelSR Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

t8y8 commented Sep 15, 2016

Uh oh!

MiguelSR commented Sep 15, 2016

Uh oh!

benlower commented Sep 17, 2016

Uh oh!

graysonarts commented Sep 19, 2016

Uh oh!

MiguelSR commented Sep 19, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MiguelSR commented Sep 8, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

t8y8 Sep 8, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.