Skip to content

fix regression in lib/clean_number (alternate) #1185

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 22, 2016
Merged

fix regression in lib/clean_number (alternate) #1185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f98edd
fix regression in lib/clean_number (alternate)
etpinard Nov 21, 2016
79fe7d5
turn front/end/global regex's into 1
etpinard Nov 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/lib/clean_number.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,16 @@ var isNumeric = require('fast-isnumeric');

var BADNUM = require('../constants/numerical').BADNUM;

// precompile these regex's for speed
var FRONTJUNK = /^['"%,$#\s']+/;
var ENDJUNK = /['"%,$#\s']+$/;
// precompile for speed
var JUNK = /^['"%,$#\s']+|[, ]|['"%,$#\s']+$/g;

/**
* cleanNumber: remove common leading and trailing cruft
* Always returns either a number or BADNUM.
*/
module.exports = function cleanNumber(v) {
if(typeof v === 'string') {
v = v.replace(FRONTJUNK, '').replace(ENDJUNK, '');
v = v.replace(JUNK, '');
}

if(isNumeric(v)) return Number(v);
Expand Down
9 changes: 7 additions & 2 deletions test/jasmine/tests/lib_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1522,7 +1522,12 @@ describe('Test lib.js:', function() {
['-100.001', -100.001],
[' $4.325 #%\t', 4.325],
[' " #1" ', 1],
[' \'\n \r -9.2e7 \t\' ', -9.2e7]
[' \'\n \r -9.2e7 \t\' ', -9.2e7],
['1,690,000', 1690000],
['1 690 000', 1690000],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson

From https://en.wikipedia.org/wiki/Decimal_mark

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so maybe we should also include ' and - (or is that a \cdot)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

['2 2', 22],
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

['$5,162,000.00', 5162000],
[' $1,410,000.00 ', 1410000],
Copy link
Member

@chriddyp chriddyp Nov 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

another one: " - " to whatever it was before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here's the old replace call

c = c.toString().replace(/['"%,$# ]/g, '');

we didn't do anything to -

].forEach(function(v) {
expect(Lib.cleanNumber(v[0])).toBe(v[1], v[0]);
});
Expand All @@ -1531,7 +1536,7 @@ describe('Test lib.js:', function() {
it('should not accept other objects or cruft in the middle', function() {
[
NaN, Infinity, -Infinity, null, undefined, new Date(), '',
' ', '\t', '2 2', '2%2', '2$2', {1: 2}, [1], ['1'], {}, []
' ', '\t', '2\t2', '2%2', '2$2', {1: 2}, [1], ['1'], {}, []
].forEach(function(v) {
expect(Lib.cleanNumber(v)).toBeUndefined(v);
});
Expand Down