floere / phony

E164 international phone number normalizing, splitting, formatting.
http://florianhanke.com/phony/
MIT License
1k stars 226 forks source link

Normalizing a country code on its own gives Phony::NationalCode inspect output #44

Closed theirishpenguin closed 12 years ago

theirishpenguin commented 12 years ago

How to reproduce issue

Phony.normalize('1')

Expected result

'1'

Actual result

"1[#<Phony::NationalCode:0x982504c @national_splitter=#<Phony::NationalSplitters::Fixed:0x9825254 @size=3, @zero=nil>, @local_splitter=#<Phony::LocalSplitters::Fixed:0xa7e6be8 @format=[3, 14]>, @normalize=true>]"

Note, I'm using version 1.7.2.

Thanks for a great gem, Declan

floere commented 12 years ago

Hi Declan,

Thanks for the sensible issue entry. I discuss this issue in https://github.com/floere/phony/issues/37 and also https://github.com/floere/phony/issues/35. Checking E164 plausibility is discussed here: https://github.com/floere/phony/blob/master/README.textile#plausibility

In a nutshell: Phony only handles international numbers that conform to E164 or are normalizable to it. For example: https://github.com/floere/phony/blob/master/spec/lib/phony_spec.rb#L190-194.

Currently, I am debating whether to raise a useful error, but since I'd like Phony to be only for handling E164 numbers, I'm unsure.

One idea is to check the number first, something like:

Phony.normalize! number if Phony.plausible? number

Does that help?

theirishpenguin commented 12 years ago

Thanks for the quick feedback floere.

I did not give the best example, which also illustrates the problem with plausible?(). Here we go, using '353' instead of '1'...

Phony.normalize! '353' if Phony.plausible? '353'

Giving the output...

"353[#<Phony::NationalCode:0x9d1a318 @national_splitter=#<Phony::
NationalSplitters::Variable:0x9d1acb4 @size=nil, @zero=\"0\",.
@mapped_ndc_min_length=1, @mapped_ndc_max_length=2, @ndcs={1=>[\"1\"], 2=>[\"21\",.
\"41\", \"42\", \"46\", \"49\", \"56\", \"58\", \"59\", \"65\", \"66\", \"71\",
\"74\", \"76\", \"90\", \"94\"]}>, @local_splitter=#<Phony::LocalSplitters::
Fixed:0x9d1a598 @format=[3, 14]>, @normalize=true>, #<Phony::NationalCode:0x9d198dc
@national_splitter=#<Phony::NationalSplitters::Variable:0x9d19e54 @size=nil,.
@zero=\"0\", @mapped_ndc_min_length=2, @mapped_ndc_max_length=3, @ndcs={2=>[\"22\",
\"23\", \"24\", \"25\", \"26\", \"27\", \"28\", \"29\", \"43\", \"44\", \"47\", \"48\",
\"52\", \"53\", \"54\", \"55\", \"62\", \"63\", \"64\", \"67\", \"68\", \"69\", \"93\",
\"94\", \"95\", \"96\", \"97\", \"98\", \"99\"], 3=>[\"402\", \"404\", \"502\", \"504\",
\"505\", \"506\", \"509\"]}>, @local_splitter=#<Phony::LocalSplitters::Fixed:0x9d19904
@format=[15]>, @normalize=true>, #<Phony::NationalCode:0x9d14cd8 @national_splitter=
#<Phony::NationalSplitters::Variable:0x9d1514c @size=nil, @zero=\"0\",
@mapped_ndc_min_length=2, @mapped_ndc_max_length=2, @ndcs={2=>[\"45\", \"51\", \"61\",
\"91\"]}>, @local_splitter=#<Phony::LocalSplitters::Fixed:0x9d14cec @format=[16]>,.
@normalize=true>, #<Phony::NationalCode:0x9d13fcc @national_splitter=#<Phony::
NationalSplitters::Variable:0x9d14b34 @size=nil, @zero=\"0\", @mapped_ndc_min_length=3,.
@mapped_ndc_max_length=3, @ndcs={3=>[\"800\"]}>, @local_splitter=#<Phony::
LocalSplitters::Fixed:0x9d14cec @format=[16]>, @normalize=true>, #<Phony::NationalCode:
0x9d13ab8 @national_splitter=#<Phony::NationalSplitters::Regex:0x9d13c70 @size=nil,.
@zero=\"0\", @regex=/^(8\\d).+$/>, @local_splitter=#<Phony::LocalSplitters::Fixed:
0x9d1a598 @format=[3, 14]>, @normalize=true>, #<Phony::NationalCode:0x9d13748
@national_splitter=#<Phony::NationalSplitters::None:0x9d1384c>, @local_splitter=#<
Phony::LocalSplitters::Fixed:0x9d1375c @format=[10]>, @normalize=true>]"

Basically I think that plausible?() is returning true but then normalize!() chokes on the processing.

floere commented 12 years ago

Thanks for the feedback. I understand the real problem now – plausible? is not returning false, as it should, but true. I will look into it why that is so. Cheers!

floere commented 12 years ago

@theirishpenguin Thanks Declan. A fix is released in 1.7.3. Also see https://github.com/floere/phony/wiki/Contributors.

theirishpenguin commented 12 years ago

That's great floere! I've retested the fix and it works great!

Thanks for the mention in the contributors file.

Agus go raibh míle maith agat :-) Declan

floere commented 12 years ago

Great to hear! Due to the nature of its domain, Phony is a work in progress and I am very glad to get such good feedback to improve it. I am the last man to say it's finished or perfect, so your help is much appreciated.

Contributors: My pleasure – if you wish to be listed in the "Github contributors", just send your next bug report as a failing test in the tests. (Although I hope that there are no further bugs) Also, if you find an Irish number that does not look ok, or if you happen to hear about changes in the Irish phone number system, I am very glad to hear about it here, as a pull request, or on twitter under "@hanke".

Thanks! I really love languages, Gaeilge is in my top 10 :) Florian

theirishpenguin commented 12 years ago

Oops. Didn't see this message you left 2 months ago. Thanks again for all the work on this and I'll shout if I hear of any updates to the Irish phone number system (hopefully there won't be any for a while!).

Gaeilge is a great ol language. Though Ruby is my fav these days :)

Ciao, Dec

floere commented 12 years ago

Hi Declan,

Thanks for the message!

Cheers, Florian