fix regression in lib/clean_number (alternate) #1185
Conversation
- allow , and spaces in middle of numeric strings
| [' \'\n \r -9.2e7 \t\' ', -9.2e7] | ||
| [' \'\n \r -9.2e7 \t\' ', -9.2e7], | ||
| ['1,690,000', 1690000], | ||
| ['1 690 000', 1690000], |
There was a problem hiding this comment.
There was a problem hiding this comment.
so maybe we should also include ' and - (or is that a \cdot)?
There was a problem hiding this comment.
I'd vote to not do any more replacements than we did previously, we'll start to hit other issues if we try to accept other separators like that automatically in every number string we see. We can build tools into the workspace that help people with these situations but lets keep them out of plotly.js
| [' \'\n \r -9.2e7 \t\' ', -9.2e7], | ||
| ['1,690,000', 1690000], | ||
| ['1 690 000', 1690000], | ||
| ['2 2', 22], |
There was a problem hiding this comment.
Oh good call - '1 690 000' will show up. '2 2' looks weird, and likely to be a parsing error, but I don't see how we can distinguish it from legitimate spaces in numbers.
| v = v | ||
| .replace(FRONTJUNK, '') | ||
| .replace(ENDJUNK, '') | ||
| .replace(GLOBALJUNK, ''); |
There was a problem hiding this comment.
The perf tests I did when playing with dates make me think it'll be faster to turn this into a single regex, but we should test that intuition, since this is a very hot path.
Also do we really want to allow any and all internal whitespace? Including line breaks and tabs? Or can we do just regular spaces?
There was a problem hiding this comment.
The perf tests I did when playing with dates make me think it'll be faster to turn this into a single regex
Good call. I'll update this.
Also do we really want to allow any and all internal whitespace?
Just ' ' is probably enough. It is also what we used to do here:
- and allow only ' ' in middle instead of all space characters
etpinard
force-pushed
the
clean-datum-regression-alt
branch
from
f804b64 to
79fe7d5
Compare
| ['1 690 000', 1690000], | ||
| ['2 2', 22], | ||
| ['$5,162,000.00', 5162000], | ||
| [' $1,410,000.00 ', 1410000], |
There was a problem hiding this comment.
another one: " - " to whatever it was before
There was a problem hiding this comment.
here's the old replace call
c = c.toString().replace(/['"%,$# ]/g, '');we didn't do anything to -
|
I'd say 💃 to this one, and close #1184 |
fixes #1183
An alternate take on #1184