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

[bugfix] prevent d3-format from raising #6386

Merged
merged 1 commit into from
Nov 14, 2018

Conversation

mistercrunch
Copy link
Member

Since #6287 and
effectively moving to a new version of d3, d3-format and d3-time-format
raises when receiving invalid input strings.

This code wraps the potential issues inside try blocks that will
effectively return an ERROR string as output to the formatting
function.

@kristw

Since apache#6287 and
effectively moving to a new version of d3, d3-format and d3-time-format
raises when receiving invalid input strings.

This code wraps the potential issues inside `try` blocks that will
effectively return an `ERROR` string as output to the formatting
function.
@codecov-io
Copy link

codecov-io commented Nov 13, 2018

Codecov Report

Merging #6386 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6386   +/-   ##
=======================================
  Coverage   77.31%   77.31%           
=======================================
  Files          67       67           
  Lines        9581     9581           
=======================================
  Hits         7408     7408           
  Misses       2173     2173

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 cb55668...5b668bd. Read the comment docs.

@kristw
Copy link
Contributor

kristw commented Nov 14, 2018

Thanks for the fix. I am also addressing this in the @superset-ui/number-format package I am working on. This PR should help with master stability meanwhile.

BTW, if the format is invalid (e.g. .abcx). Should we still show the raw value?
e.g. 12345 (Invalid format: .abcx) instead of ERROR

@mistercrunch
Copy link
Member Author

About ERROR, I wanted to make sure that users get feedback that the format string that they authored is not working, as opposed to just doing some sort of a false positive feedback.

@mistercrunch
Copy link
Member Author

@kristw did a bit of thinking about "the proper way" here, and ideally the control itself would do the validation and provide much more context to the user. I'm thinking even showing a small table of source number to formatted number mapping that shows the user as they select or type new format strings.

For now though, I'm just trying to ship a version of Superset that doesn't crash.

@kristw
Copy link
Contributor

kristw commented Nov 14, 2018

sgtm. I just post a PR for the number-format package FYI.
apache-superset/superset-ui#31

@mistercrunch
Copy link
Member Author

mistercrunch commented Nov 14, 2018

Ok nice, we'll probably need similar for time formatting. In the meantime, should we merge this as master is broken?

@kristw
Copy link
Contributor

kristw commented Nov 14, 2018

Yes, please merge.

@mistercrunch mistercrunch merged commit 4690563 into apache:master Nov 14, 2018
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request Nov 20, 2018
Since apache#6287 and
effectively moving to a new version of d3, d3-format and d3-time-format
raises when receiving invalid input strings.

This code wraps the potential issues inside `try` blocks that will
effectively return an `ERROR` string as output to the formatting
function.
(cherry picked from commit 4690563)
mistercrunch added a commit that referenced this pull request Nov 30, 2018
Since #6287 and
effectively moving to a new version of d3, d3-format and d3-time-format
raises when receiving invalid input strings.

This code wraps the potential issues inside `try` blocks that will
effectively return an `ERROR` string as output to the formatting
function.

(cherry picked from commit 4690563)
bipinsoniguavus pushed a commit to ThalesGroup/incubator-superset that referenced this pull request Dec 26, 2018
Since apache#6287 and
effectively moving to a new version of d3, d3-format and d3-time-format
raises when receiving invalid input strings.

This code wraps the potential issues inside `try` blocks that will
effectively return an `ERROR` string as output to the formatting
function.
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 2024
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
Since apache#6287 and
effectively moving to a new version of d3, d3-format and d3-time-format
raises when receiving invalid input strings.

This code wraps the potential issues inside `try` blocks that will
effectively return an `ERROR` string as output to the formatting
function.

(cherry picked from commit b5238c9)
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 v0.29 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants