Skip to content

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ilyichv
Copy link

@ilyichv ilyichv commented Jul 7, 2025

Add support for any value within isPassportNumber as done for isPostalCode

isPassportNumber("AB1234567", "any")

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

Copy link

codecov bot commented Jul 7, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.96%. Comparing base (72573b3) to head (2540ebc).

Files with missing lines Patch % Lines
src/lib/isPassportNumber.js 85.71% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

for (const key in passportRegexByCountryCode) {
if (passportRegexByCountryCode.hasOwnProperty(key)) {
const regex = passportRegexByCountryCode[key];
if (regex.test(normalizedStr)) {
Copy link
Contributor

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?

Copy link
Author

@ilyichv ilyichv Jul 8, 2025

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));
}

Copy link
Contributor

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 :-)

Copy link
Author

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!

Copy link
Contributor

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

Copy link
Author

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 :)

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.

2 participants