Skip to content

Fix wrong region code uses#41

Merged
marmelroy merged 4 commits intomarmelroy:masterfrom
SixiemeEtage:fix-wrong-region-code-uses
Apr 29, 2016
Merged

Fix wrong region code uses#41
marmelroy merged 4 commits intomarmelroy:masterfrom
SixiemeEtage:fix-wrong-region-code-uses

Conversation

@dulacp
Copy link
Copy Markdown
Contributor

@dulacp dulacp commented Apr 5, 2016

Our number validator is getting some weird errors as we expand in other countries.

The numbers are validated on the phone, then if valid transmitted to our backend where an extra step of validation is run. And the backend step is triggering a lot of minor validation issues (mainly due to an incompatible combination between a valid number and a wrong region code).

So I've implemented some failing test for real usage cases that happened for us.
First step toward solving this and making this great project even more robust !

Cheers

@dulacp
Copy link
Copy Markdown
Contributor Author

dulacp commented Apr 5, 2016

I'm seeing that the resulting PhoneNumber instance has a type of Unknown.
Shouldn't it raise an exception ?

@marmelroy
Copy link
Copy Markdown
Owner

Thanks a lot for these @dulaccc !

This issue was discussed a little bit in #30.

Basically, the one major check that Google's libPhoneNumber does that PhoneNumberKit doesn't is a type check. Unknown types are considered possible but invalid numbers.

The issue is that in order to determine a phone number's type, MANY regular expressions are necessary (essentially comparing the number to every known type until there is a fit). This slows things down considerably and is not a behaviour I wanted to enable by default.

I'll resolve this issue in the next release.

@dulacp
Copy link
Copy Markdown
Contributor Author

dulacp commented Apr 5, 2016

Ok! Thank you for the very clear explanation. So my tests are not implemented correctly.
Now I see that type is a computed property ! it all makes sense for performance purposes, my bad.

I will keep on posting if I have unexpected behaviors in the coming weeks.
Feel free to close this issue if you need to, and thanks for this very well structured project !

@marmelroy marmelroy merged commit 71827b0 into marmelroy:master Apr 29, 2016
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