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 python2 str() in visualization #5093

Merged
merged 1 commit into from
May 29, 2018
Merged

Fix python2 str() in visualization #5093

merged 1 commit into from
May 29, 2018

Conversation

zjj
Copy link

@zjj zjj commented May 29, 2018

refers to 3ed8f5f by timifasubaa

@codecov-io
Copy link

codecov-io commented May 29, 2018

Codecov Report

Merging #5093 into master will not change coverage.
The diff coverage is 33.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5093   +/-   ##
=======================================
  Coverage   77.53%   77.53%           
=======================================
  Files          44       44           
  Lines        8710     8710           
=======================================
  Hits         6753     6753           
  Misses       1957     1957
Impacted Files Coverage Δ
superset/viz.py 81.42% <33.33%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c18ef89...fc2737f. Read the comment docs.

@xrmx
Copy link
Contributor

xrmx commented May 29, 2018

One nice way to fix these is by importing from builtins import str so that python2.7 will use str as unicode string instead of byte string.

@zjj
Copy link
Author

zjj commented May 29, 2018

I found this issue while using python2.7, don't use str here. actually, I think it's not right to use str() here even with python2.7, eg, utf8 bytes, chinese characters will raise exception. @xrmx

the origin patch was

-                series_title = [str(title) for title in name]
+                series_title = [title for title in name]
             elif isinstance(name, tuple):
-                series_title = tuple(str(title) for title in name)
+                series_title = tuple(title for title in name)
             else:
-                series_title = str(name)
+                series_title = name

and run gunicore with PYTHONIOENCODING="utf8" to fix that.

and I git log the history, found the refered commit. and that way seems easy for guys to handle.

@xrmx
Copy link
Contributor

xrmx commented May 29, 2018

@zjj
Copy link
Author

zjj commented May 29, 2018

@xrmx
ah, I misunderstand your reply, I am not familiar with python3 and I doubt the origin patch that removed 'str()' would do something stupid with python3, so there comes the six.text_type for both python3 and python2.

@mistercrunch mistercrunch merged commit 4592677 into apache:master May 29, 2018
timifasubaa pushed a commit to timifasubaa/incubator-superset that referenced this pull request May 31, 2018
timifasubaa pushed a commit to airbnb/superset-fork that referenced this pull request Jul 25, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.26.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.26.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants