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

Phony.normalize exception on '000000' and 'hello' #37

Closed
glebm opened this issue Mar 14, 2012 · 6 comments
Closed

Phony.normalize exception on '000000' and 'hello' #37

glebm opened this issue Mar 14, 2012 · 6 comments

Comments

@glebm
Copy link
Contributor

glebm commented Mar 14, 2012

Phony.normalize('0000000')

=> NoMethodError: undefined method `normalize' for 1:Fixnum

Phony.normalize('hello')

=> NoMethodError: undefined method `normalize' for 1:Fixnum

@floere
Copy link
Owner

floere commented Mar 14, 2012

Thanks for reporting. What was your expectation?

@glebm
Copy link
Contributor Author

glebm commented Mar 15, 2012

Either a more sensible error message or nil (even better)

@floere
Copy link
Owner

floere commented Mar 16, 2012

Since I don't want Phony to become the Swiss knife of telephone numbers, the behavior of this method is actually undefined if anything else than a E164 normalizable number is passed in, meaning a number that does not contain enough information to turn it into a E164 number. "+41/44+555///12...34" would be normalizable, while "00000" is not.

When I wrote it, nil sounded very tempting initially. However, that would require me to catch all errors and just return nil instead, losing information in the process that wouldn't be available anymore to the user.

However, in another issue #35 I am working on adding a plausible? / conforms? / valid? method to Phony, which is geared towards checking numbers for plausibility. In combination with that one, returning nil would be more sensible.

Why is the combination important? Currently, if you normalize an almost-E164 number, it will not fail. It only fails if it is too short or too un-numerical. So actually, normalize will fail only sometimes. Again, the behavior is simply undefined. And it's not a method that can be used to test for ok-ness, just for normalizing noisy E164 numbers.

I need to think about this one a bit. If you have some good ideas, please let me know. If not, I will write again here if I make progress. In the meantime, please rescue.

@floere
Copy link
Owner

floere commented Mar 18, 2012

I've just released version 1.6.6 which includes an experimental Phony.plausible?(number) method to check if the given number is a plausible E164 number. Cheers! Your examples return false.

@glebm
Copy link
Contributor Author

glebm commented Mar 21, 2012

Thanks, I can now deploy phony to production :)

@floere
Copy link
Owner

floere commented Mar 21, 2012

I'm glad to hear it, Gleb! Please update to the latest version please. Version 1.6.6 is too strict regarding plausibility of numbers.

Closing this now – please reopen or open another issue should there be more.

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

2 participants