Skip to content
This repository has been archived by the owner on Feb 10, 2019. It is now read-only.

Extended phone regex - see below #82

Merged
merged 3 commits into from Aug 30, 2013
Merged

Extended phone regex - see below #82

merged 3 commits into from Aug 30, 2013

Conversation

u01jmg3
Copy link
Contributor

@u01jmg3 u01jmg3 commented Aug 21, 2013

Added ^ and $ for start and end so surplus chars cause an error
Extended the numbers allowed in brackets to be 6 so (01224) is allowed
Added the optional space so ext 5555 is allowed as well as ext5555
Made the whole thing case insensitive to allow for EXTENSION, etc

Added ^ and $ for start and end so surplus chars cause an error
Extended the numbers allowed in brackets to be 6 so (01224) is allowed
Added the optional space so ext 5555 is allowed as well as ext5555
Made the whole thing case insensitive to allow for EXTENSION
@ericelliott
Copy link
Owner

Added ^ and $ for start and end so surplus chars cause an error

This is automatically implied. Please remove this part.

Extended the numbers allowed in brackets to be 6 so (01224) is allowed

Could you give me an example of a real-world number that uses that?

Added the optional space so ext 5555 is allowed as well as ext5555

This is great. Thanks!

Made the whole thing case insensitive to allow for EXTENSION, etc

This change is not compatible with the code that ingests the patterns. Consider switching the or block to [a-zA-Z], instead.

Removed ^ and $
Added a-z to the block
Included uppercase letters for x, ext and extension to make it case
insensitive for only this part
@u01jmg3
Copy link
Contributor Author

u01jmg3 commented Aug 22, 2013

Removed ^ and $ as asked

A real-world example would be someone entering their phone number as "(01224) 271234" as it's implied that the area code is common knowledge so is included in brackets.

No problem

Removed /.../i to comply with your code: see amended commit

@ericelliott
Copy link
Owner

([Xx]|[Ee][Xx][Tt]|[Ee][Xx][Tt][Ee][Nn][Ss][Ii][Oo][Nn])

I'd prefer to allow any text, using the ranges that I showed before. It's less code, and it's more flexible, too.

Could you add some tests for these new use cases? =)

Thanks again. I really appreciate the effort.

Changed ([Xx]|[Ee][Xx][Tt]|[Ee][Xx][Tt][Ee][Nn][Ss][Ii][Oo][Nn]) to be a
block: [A-Za-z \:]{1,11}
Added 10 test cases
@u01jmg3
Copy link
Contributor Author

u01jmg3 commented Aug 22, 2013

Agreed - much cleaner

See what you think of my latest commit

@u01jmg3
Copy link
Contributor Author

u01jmg3 commented Aug 27, 2013

Any update?

@ericelliott
Copy link
Owner

This is much improved. I have not had time to pull it down and run the test suite. I've been very busy this past week.

@u01jmg3
Copy link
Contributor Author

u01jmg3 commented Aug 28, 2013

No problem - sorry to hassle. I'll wait until you're free to close this :)

@ericelliott
Copy link
Owner

Please don't close it. I'll test, merge, and close as soon as I have the
time.

Author - "Programming JavaScript Applications" (O'Reilly 2012)
Celebrity Music Photographer - We Are Photographers Council Member / Judge

http://bit.ly/Hx22vN

On Tue, Aug 27, 2013 at 5:16 PM, u01jmg3 notifications@github.com wrote:

No problem - sorry to hassle. I'll wait until you're free to close this :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/82#issuecomment-23381450
.

@u01jmg3
Copy link
Contributor Author

u01jmg3 commented Aug 28, 2013

Yes, my intention is to wait

ericelliott pushed a commit that referenced this pull request Aug 30, 2013
Extended phone regex - see below
@ericelliott ericelliott merged commit afdbe79 into ericelliott:master Aug 30, 2013
@ericelliott
Copy link
Owner

Looks good.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants