-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(isPassportNumber): add support for any #2570
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2570 +/- ##
===========================================
- Coverage 100.00% 99.96% -0.04%
===========================================
Files 114 114
Lines 2535 2542 +7
Branches 641 644 +3
===========================================
+ Hits 2535 2541 +6
- Partials 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
for (const key in passportRegexByCountryCode) { | ||
if (passportRegexByCountryCode.hasOwnProperty(key)) { | ||
const regex = passportRegexByCountryCode[key]; | ||
if (regex.test(normalizedStr)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the if statement here is not needed: regex.test returns a boolean.
Why not just directly return that value, instead of manually checking if it is "true-ish" and then returning true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning directly the result won't work since it breaks the loop and you want to check if at least one element matches.
I tried to follow the code style of isPostalCode
. A better-looking version could be:
if (countryCode === 'any') {
return Object.values(passportRegexByCountryCode).some(regex => regex.test(normalizedStr));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, of course, you're right – I overlooked that we are in a for loop, my bad!
I do like your new version though :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code an then noticed that you're testing node 6 as well and of course it fails. If you agree we can keep the "dirty" version and move on!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot about the CI testing against Node 6 (which IMHO is useless that it still is done, seeing that it was EOL'ed almost 6 years ago :-()
so yeah, all good to go back with your original version then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a bit weird! I reverted the latest version anyway :)
This reverts commit 473767e.
Add support for
any
value withinisPassportNumber
as done forisPostalCode
Checklist