-
-
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
feat: (isMobilePhone) Add mobile phone validation for Cameroon (fr-CM) #1772
Conversation
I have added a regex for validating Cameroonian mobile numbers which should work. I'm not super comfortable with regular expressions, so if I have screwed something up, please let me know. I have done some basic testing and it has functioned fine thus far.
Fixing the commas now. My bad! |
Fix my sloppy tests. I'm really tired and probably should not be working on this, but I accidentally added an extra digit while typing.
Codecov Report
@@ Coverage Diff @@
## master #1772 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 102 102
Lines 2052 2052
Branches 463 463
=========================================
Hits 2052 2052
Continue to review full report at Codecov.
|
I apologize for the mistakes that I've made while doing this; I'm really anxious that I've messed something critical up. |
No worries, tests and linter are here to fix those mistakes. I suggest next time you run them locally to detect that kind of problems, you just need to run |
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.
Thank you for your PR @beckettnormington !
I Added a comment concerning your regex, can you address it?
src/lib/isMobilePhone.js
Outdated
@@ -74,6 +74,7 @@ const phones = { | |||
'fi-FI': /^(\+?358|0)\s?(4(0|1|2|4|5|6)?|50)\s?(\d\s?){4,8}\d$/, | |||
'fj-FJ': /^(\+?679)?\s?\d{3}\s?\d{4}$/, | |||
'fo-FO': /^(\+?298)?\s?\d{2}\s?\d{2}\s?\d{2}$/, | |||
'fr-CM': /^((237) ?|(\+237) ?)([0-9] ?){9}$/, |
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.
Looks like you want to validate country prefix with and without the +
sign. An easier way to do this would be:
/^(\+?237)? ([0-9] ?){9}$/
I have two questions tho:
- Are spaces valid in Cameroon mobile phones (Most of the mobile phone validators we have don't allow spaces)
- There is some carrier prefixes for mobile phone numbers as shown here. Can you please update your regex to take into consideration that mobile prefix?
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.
Thanks for the feedback! I'm currently writing a regex that takes your comments into account.
1.) Spaces don't seem to be valid, my bad.
2.) I'm taking the latest updated mobile prefix (6) from November 21, 2014. If that needs to be updated, let me know as I wasn't completely sure.
I'll update the tests to invalidate landline numbers.
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.
Alright, done. 😄
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.
Thank you @beckettnormington, looks like you still have a space between the country prefix and the local part of the phone number. Can you correct that and we should be good to go
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.
Whoops, completely forgot about that! I’ll fix that now. My bad!
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.
Fixed.
Add mobile prefix (6) to regex, remove space allowance, and optimize regex
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.
LGTM ! Thank you 🎉
@beckettnormington -- pls fix the merge conflict on README and we should be good to go. |
Resolved merge conflict, @profnandaa. |
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.
LGTM. Thank you for fixing merge conflicts
* Add Cameroon validation regex I have added a regex for validating Cameroonian mobile numbers which should work. I'm not super comfortable with regular expressions, so if I have screwed something up, please let me know. I have done some basic testing and it has functioned fine thus far. * Update README to include fr-CM in isMobilePhone * Add (very) basic testing for Cameroonian mobile number * Fix missing brace (whoops!) * Fix commas (I hope) * Fix sloppy tests Fix my sloppy tests. I'm really tired and probably should not be working on this, but I accidentally added an extra digit while typing. * Update regex for correctness Add mobile prefix (6) to regex, remove space allowance, and optimize regex * Update tests for correctness * Remove invalid space Sorry! * Rerun failed tests (internal server error)
* Add Cameroon validation regex I have added a regex for validating Cameroonian mobile numbers which should work. I'm not super comfortable with regular expressions, so if I have screwed something up, please let me know. I have done some basic testing and it has functioned fine thus far. * Update README to include fr-CM in isMobilePhone * Add (very) basic testing for Cameroonian mobile number * Fix missing brace (whoops!) * Fix commas (I hope) * Fix sloppy tests Fix my sloppy tests. I'm really tired and probably should not be working on this, but I accidentally added an extra digit while typing. * Update regex for correctness Add mobile prefix (6) to regex, remove space allowance, and optimize regex * Update tests for correctness * Remove invalid space Sorry! * Rerun failed tests (internal server error)
I have added a regex for validating Cameroonian mobile numbers which should work. I'm not super comfortable with regular expressions, so if I have screwed something up, please let me know and I will (hopefully) be able to fix it.
Checklist