-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Conversation
FYI, I tried running the tests and there are failing tests that are unrelated to this PR. They mostly have to do with the I can fix those in this pull request as well if you want, or I could create a new pull request for those. |
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 |
Either would be great, thanks! |
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. Also, just so I understand the historical reasons for this, what was the initial justification for not accepting leading zeroes? |
To fix the broken pre-tests I disabled some 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. |
The regular expression in the very first commit of the library was 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('10.0')
10
> parseInt('10foo')
10 Although it might be useful to check whether the input string is a canonical int, e.g. |
Disabling the |
One more thing: can you update the README to show the |
Great, thanks. |
I updated the
isInt()
method so it treats numeric strings with prepended zeroes as valid integers. Since theparseInt()
function in JavaScript properly parses zero-prepended strings, it would make sense that theisInt()
function would support it as well.I'm curious if it might be better for the
isInt()
function to check for integer values by usingparseInt()
and checking whether it returns NaN. That way it's ensured that theisInt()
method will always validate integers that can properly be parsed bytoInt()
(or rather, byparseInt()
by proxy).