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

inconsistent normalization #93

Closed
krukgit opened this issue May 11, 2015 · 6 comments
Closed

inconsistent normalization #93

krukgit opened this issue May 11, 2015 · 6 comments

Comments

@krukgit
Copy link
Contributor

krukgit commented May 11, 2015

Sample uses of normalization functions:

> '81234567'.phony_normalized country_code: 'SG'
 => "+6581234567"
> PhonyRails.normalize_number '81234567', country_code: 'SG'
 => "+6581234567"
> Phony.normalize '81234567', cc: '65'
 => "6581234567" 
> '81234567'.phony_normalized country_code: 'SG', add_plus: false
 => "+6581234567"
> PhonyRails.normalize_number '81234567', country_code: 'SG', add_plus: false
 => "6581234567"
> PhonyRails.normalize_number '81234567', add_plus: true
 => "+81234567"

There are a few problems:

  1. String#phony_normalized's behaviour is not consistent with PhonyRails.normalize_number
  2. add_plus doesn't verify whether the number includes a country code
  3. add_plus defaults to true while Phony#split and Phony#format don't work with pluses

ad 1.
Should be easily fixed by passing all options to PhonyRails.normalize_number instead of just country_code

ad 2.
PhonyRails should check whether phone number is Phony.plausible? before appending +

ad 3.
What's the rationale behind add_plus defaulting to true? Is it to make clear the numbers are international? While it makes perfect sense, I believe it should default to false for the sake of compatibility with Phony. It should at least be well documented on the front page.

Let me know whether you agree with my comments and I can submit pull requests for all three points.

@krukgit
Copy link
Contributor Author

krukgit commented May 11, 2015

ad 2.
PhonyRails.normalize_number with country_code option may generate an incorrect phone number, so Phony.plausible? will fail even if the country code is appended.
It also seems Phony.plausible? is not correct for all countries, so relying on it before adding + would make a mess off the database.

@joost
Copy link
Owner

joost commented May 11, 2015

Regarding nr 3: it indeed is to 'know' we have a country code. There are some edge cases for which it isn't clear if the first digits is a country-code or just the plain number.
This way we differentiate between those.

@joost
Copy link
Owner

joost commented May 11, 2015

Improvement regarding 2 welcome :)

@krukgit
Copy link
Contributor Author

krukgit commented May 11, 2015

I'll have to think about it. I wrote the code but then I realized it might be problematic if phone number does have a country code but is not being recognized as correct. It's ok if we assume Phony is infallible and undefined behaviour for incorrect phone numbers is fine but it might be better to just leave the responsibility in the hands of users (in that situation we need to add add_plus to the docs).

The code below. It breaks two tests.
krukgit@0eed62f

@joost
Copy link
Owner

joost commented May 11, 2015

Thx. I like to leave it in the hands of the user indeed :)
Will close the issue for now. Lets improve if you think it is 100% :)

@joost joost closed this as completed May 11, 2015
@allaire
Copy link

allaire commented Aug 5, 2016

@joost Just got bit by add_plus being true by default.

Show we default add_plus to false so it's 100% compatible with Phony?

Otherwise, we should at least document it in the README

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

No branches or pull requests

3 participants