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

Make sure the phone number consists of only digits #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zsyed91
Copy link
Contributor

@zsyed91 zsyed91 commented Jan 22, 2016

I saw this TODO and thought this would be straight forward to tackle. I was thinking of also adding a lower and upper bound in length. Researching phone number lengths made it seem like there is an international standard of 15 - 16 digits. We could increase this to maybe 20 as a catch all. What are your thoughts?

Valid formats as of now are only digits so 1234567 or 18005551234 etc.

Also, I did not see anywhere to add/update tests to check this code change. Any pointers regarding that, feels dirty not to have a test for this change 😄

@pjf
Copy link
Owner

pjf commented Jan 25, 2016

Oh gosh, phone numbers are tricky. '+' should definitely be allowed as a prefix, as that allows one to specify country. It's nice to be able to allow other human-friendly formatting characters like parentheses, spaces, or minus signs, although we can and probably should strip these as part of normalisation. So ^\+?\d+$ is probably a good type-check for a phone-number (the anchors at the start/end are important!), and if we wanted to get fancy then it should be possible to use a coerce PhoneNum from Str via ... to normalise from human-friendly strings.

Also, I did not see anywhere to add/update tests to check this code change. Any pointers regarding that, feels dirty not to have a test for this change.

You totally rock. I'd suggest your choice of t/types.t or t/types/phone.t (both of which would need to be created).

Than you again for this!

@zsyed91
Copy link
Contributor Author

zsyed91 commented Jan 25, 2016

Awesome, thanks for the feedback. I will go ahead and implement the changes and add some tests. Should hopefully get some time this week to tackle this.

@waldyrious
Copy link

Perhaps libphonenumber could be useful (live demo)

@zsyed91
Copy link
Contributor Author

zsyed91 commented Feb 1, 2016

I haven't forgotten about this, it will get done 😄

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.

3 participants