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 regression in lib/clean_number #1184

Closed
wants to merge 2 commits into from
Closed

Conversation

etpinard
Copy link
Contributor

- junk in the 'middle' of numeric string is again accepted
@etpinard etpinard added status: reviewable bug something broken labels Nov 21, 2016
var FRONTJUNK = /^['"%,$#\s']+/;
var ENDJUNK = /['"%,$#\s']+$/;
// precompile for speed
var JUNK = /['"%,$#\s]/g;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

['2 2', 22],
['2%2', 22],
['2$2', 22],
['1,690,000', 1690000]
Copy link
Member

@chriddyp chriddyp Nov 21, 2016

Choose a reason for hiding this comment

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

could we add one for '$5,162,000.00' to 5162000?

Copy link
Member

@chriddyp chriddyp Nov 21, 2016

Choose a reason for hiding this comment

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

Also: " $1,410,000.00 " to 1410000?

['2 2', 22],
['2%2', 22],
['2$2', 22],
['1,690,000', 1690000]
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right that this is what we had before - which is exactly why I changed it, though obviously I forgot about commas in the middle! I really think we should consider interpreting '2 2', '2%2', '2$2' etc as numbers to be bugs we should fix, unless someone can show me a legitimate use case for them to be interpreted as 22.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about '1 000 000' ?

@alexcjohnson
Copy link
Collaborator

Since we're reverting this to buy us some time... lets do this right. Can we do:
var JUNK = /(^['"%,$#\s]|,|['"%,$#\s]/g
ie strip only commas in the middle, and all these other characters on the ends?

@etpinard etpinard closed this Nov 22, 2016
@etpinard etpinard deleted the clean-datum-regression branch November 22, 2016 14:35
@etpinard
Copy link
Contributor Author

Closed in favour of #1185

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

certain data input strings lead to broken plots.
3 participants