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

Allow prepended zeroes in toInt() #532

Merged
merged 4 commits into from
May 28, 2016
Merged

Conversation

theartoflogic
Copy link
Contributor

I updated the isInt() method so it treats numeric strings with prepended zeroes as valid integers. Since the parseInt() function in JavaScript properly parses zero-prepended strings, it would make sense that the isInt() function would support it as well.

I'm curious if it might be better for the isInt() function to check for integer values by using parseInt() and checking whether it returns NaN. That way it's ensured that the isInt() method will always validate integers that can properly be parsed by toInt() (or rather, by parseInt() by proxy).

@theartoflogic
Copy link
Contributor Author

FYI, I tried running the tests and there are failing tests that are unrelated to this PR. They mostly have to do with the no-control-regex lint validation in isAscii(), isEmail(), and isMultibyte().

I can fix those in this pull request as well if you want, or I could create a new pull request for those.

@chriso
Copy link
Collaborator

chriso commented May 27, 2016

This is a breaking change, so probably worth controlling it with an option and leaving the default behavior as-is, e.g.

> validator.isInt('01');
false
> validator.isInt('01', {allow_leading_zeroes: true});
true

@chriso
Copy link
Collaborator

chriso commented May 27, 2016

I can fix those in this pull request as well if you want, or I could create a new pull request for those.

Either would be great, thanks!

@theartoflogic
Copy link
Contributor Author

I'm fine with controlling this with an option if you'd rather not have to bump the major version just to account for the breaking change, but just to throw out my 2 cents on why I would dissent to that:

I feel that the default behavior of not accepting leading zeroes is a bug, not a feature that should be favored as the default. Because a value with leading zeroes is a valid integer, the default behavior should be to allow them, with the option to disallow them.

One argument against allowing leading zeroes is that in some cases they are used to denote octal values (e.g. 010 when parsed as an octal is 8 in decimal). However, I feel this is not a compelling argument since that use-case doesn't apply here as the user is explicitly passing the value to functions that deal with integers and wouldn't expect to need to disambiguate it from an octal value.

Also, just so I understand the historical reasons for this, what was the initial justification for not accepting leading zeroes?

@coveralls
Copy link

coveralls commented May 27, 2016

Coverage Status

Coverage remained the same at 99.087% when pulling f624d46 on theartoflogic:master into 0882fa4 on chriso:master.

@theartoflogic
Copy link
Contributor Author

To fix the broken pre-tests I disabled some no-control-regex eslint checks. I think that is valid for the isAscii() and isMultibyte() functions, but I'm not so sure about the isEmail() function. I don't have time right now to dissect the complex regex statements for the isEmail() function, but if you think a better route would be to update the regex statements to remove control characters from the hex ranges there then I could look into doing that too.

Although, I'm not up to speed on the evolution of those particular regex statements to validate emails, so there might be a valid reason control characters are allowed there.

@coveralls
Copy link

coveralls commented May 27, 2016

Coverage Status

Coverage increased (+0.003%) to 99.09% when pulling a3e3880 on theartoflogic:master into 0882fa4 on chriso:master.

@chriso
Copy link
Collaborator

chriso commented May 28, 2016

The regular expression in the very first commit of the library was /^(?:0|[1-9][0-9]*)$/. It was 5+ years ago so I can't recall exactly, but I don't think I made a conscious decision to reject leading zeroes, it was probably just an oversight.

I think you're right that we should accept leading zeroes by default, but since this is the first time the issue has come up in that entire time I don't think it's worth bumping a major for immediately. I've created a wiki page to collect breaking changes between v5 and v6: https://github.com/chriso/validator.js/wiki/Proposed-breaking-changes-for-v6

Also FYI parseInt is too lenient to be used in validation:

> parseInt('10.0')
10
> parseInt('10foo')
10

Although it might be useful to check whether the input string is a canonical int, e.g. input_str === parseInt(input_str).toString(), which I guess is close to what isInt() is doing now.

@chriso
Copy link
Collaborator

chriso commented May 28, 2016

Disabling the no-control-regex is fine for now, thanks.

@chriso
Copy link
Collaborator

chriso commented May 28, 2016

One more thing: can you update the README to show the allow_leading_zeroes option?

@coveralls
Copy link

coveralls commented May 28, 2016

Coverage Status

Coverage increased (+0.003%) to 99.09% when pulling 4923be5 on theartoflogic:master into 0882fa4 on chriso:master.

@chriso
Copy link
Collaborator

chriso commented May 28, 2016

Great, thanks.

@chriso chriso merged commit e7f0363 into validatorjs:master May 28, 2016
chriso added a commit that referenced this pull request May 28, 2016
chriso added a commit that referenced this pull request Sep 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants