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(isMobilePhone): update phone regex for Zambia #2037

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

Conversation

imkrishh
Copy link

@imkrishh imkrishh commented Sep 9, 2022

Updated Zambia phone number RegEx validator

Added support for new phone network provider for Zambia [ref-link]
077 - Airtel (New Serial #)
076 - MTN (New Serial #)

Checklist

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

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (f21d839) compared to base (86a07ba).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##            master     #2037    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          104       104            
  Lines         2203      2308   +105     
  Branches       477       578   +101     
==========================================
+ Hits          2203      2308   +105     
Impacted Files Coverage Δ
src/lib/isMobilePhone.js 100.00% <ø> (ø)
src/lib/isIP.js 100.00% <0.00%> (ø)
src/lib/isIn.js 100.00% <0.00%> (ø)
src/lib/trim.js 100.00% <0.00%> (ø)
src/lib/alpha.js 100.00% <0.00%> (ø)
src/lib/isBIC.js 100.00% <0.00%> (ø)
src/lib/isEAN.js 100.00% <0.00%> (ø)
src/lib/isHSL.js 100.00% <0.00%> (ø)
src/lib/isInt.js 100.00% <0.00%> (ø)
src/lib/isJWT.js 100.00% <0.00%> (ø)
... and 83 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pano9000
Copy link
Contributor

pano9000 commented Dec 8, 2022

Hi @imkrishh,

thanks for the PR.
For future reference, here's also the ITU numbering plan, which is always a more accurate reference than a Wikipedia entry:
https://www.itu.int/dms_pub/itu-t/oth/02/02/T02020000E80006PDFE.pdf

It looks like currently only the following ranges are assigned and valid, which your PR is implementing, so thanks!
75, 76, 77
95, 96, 97

Also another reference for the future:
Real world random examples from Google Maps for those of these numbers
https://goo.gl/maps/4sNtcZAWjzkZKnin7
+260 97 xxxxxxx

https://goo.gl/maps/YbE5RygVAwBarQhj7
+260 76 xxxxxxx

Justone thing, that I think still needs to be done ideally:
Could you kindly also add an additional test for one of these new numbers into the tests as well?

Thanks!


for future reference:
there is another (unrelated to this PR) issue with the en-ZM RegExp, but that is rather part of the discussion in #2124, so that shouldn't stop this PR from being merged after the updated tests are provided

Copy link
Contributor

@pano9000 pano9000 left a comment

Choose a reason for hiding this comment

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

thank you.
looks good to me now :-)
However please note, I don't have any privileges in this project, so we will have to wait for two of the maintainers to review and merge this.

@pano9000 pano9000 requested a review from profnandaa March 9, 2023 00:22
@profnandaa profnandaa added the mc-to-land Just merge-conflict standing between the PR and landing. label Jun 26, 2023
Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

LGTM. can fix the merge conflict if possible, or I'll fix them during clean-up.

@rubiin rubiin removed the ✅ LGTM label May 10, 2024
@rubiin
Copy link
Member

rubiin commented May 10, 2024

@imkrishh can you fix the merge conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 first-pr mc-to-land Just merge-conflict standing between the PR and landing. 🎉 new contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants