DrHyde / perl-modules-Number-Phone

Number::Phone and friends
24 stars 31 forks source link

Add libphonenumber 'format' field support. #80

Closed dracos closed 6 years ago

dracos commented 6 years ago

This stores the format/intlFormat given for each libphonenumber formatter in the stub country files, and adds a new NationallyPreferred formatter to use that infomation.

Fixes #78.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.07%) to 99.901% when pulling fc489598a51d75337b3f7d41d77ee7c45cd00ae4 on dracos:78-store-format-field into 850ad3185171a00c5962b73dfc169ce08f1cab9a on DrHyde:master.

dracos commented 6 years ago

The coverage drop is because every entry in the XML provides a format, but I left in the else code in case it did not. If it's okay to assume all entries will do so, then the else code can be dropped and that section simplified.

dracos commented 6 years ago

Now removed the 'else' case. I've defaulted to intlFormat given format() outputs with a +countycode, but note a few intlFormats are NA as they can't be called internationally. I don't know if you'd like something different done about them, but it does at least mean it won't output a number that wouldn't work (I imagine these are quite edge case anyway).

For #79 I'll work on adding separate new format_national and format_international functions to keep that independent.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+3.0e-05%) to 99.967% when pulling 1df809a374acedad7a0ff2661ac46b1fbd6ed20c on dracos:78-store-format-field into 850ad3185171a00c5962b73dfc169ce08f1cab9a on DrHyde:master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+3.0e-05%) to 99.967% when pulling 99eca0eff043e6ccecd646f57f146ca93b5dafad on dracos:78-store-format-field into 850ad3185171a00c5962b73dfc169ce08f1cab9a on DrHyde:master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+3.0e-05%) to 99.967% when pulling a885e59f72fdf492215cecc7ddc122591f5c1f7f on dracos:78-store-format-field into 850ad3185171a00c5962b73dfc169ce08f1cab9a on DrHyde:master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+7.0e-05%) to 99.967% when pulling f5cca3283312fd4dc31b611c51e581cce1023e8b on dracos:78-store-format-field into 850ad3185171a00c5962b73dfc169ce08f1cab9a on DrHyde:master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+7.0e-05%) to 99.967% when pulling 635219250121f72d6fbc008e2f1b256710ddc932 on dracos:78-store-format-field into 850ad3185171a00c5962b73dfc169ce08f1cab9a on DrHyde:master.

DrHyde commented 6 years ago

Sorry for taking ages to get back to you on this, Real Life intervened and then I massively slacked off over Christmas and New Year. I like the idea of storing the nationally preferred representation of a number and making it available. However, the format() method is documented as returning a number in E.123 format - see all the examples in section 2 of the standard. I think this would best be implemented as a formatter - see Number::Phone::Formatters and Number::Phone::Formatter::Raw for an example.

If this could be reworked into something like Number::Phone::Formatter::NationallyPreferred I'd be happy to merge it.

dracos commented 6 years ago

Okay, I've done that, it seems to work okay. I had to pass the object into the formatter, but kept the string argument for backwards compatibility, dos that make sense?

For the other PR that then depends upon this one (national formatting), I can add another formatter to return the national formatted number (Number::Phone::Formatter::National?), if that is okay with you, but I can't see how I can do the "pass in a country and have it pick national/international as appropriate", so that could instead be a utility function somewhere? It does seem like an obvious thing to want. Update: I've done that.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.03%) to 99.937% when pulling da17163ea0904ab395330985a4dc4b508d0d9c89 on dracos:78-store-format-field into fdabfd2d5530278f06b570c5c9c934478e10fe8f on DrHyde:master.

DrHyde commented 6 years ago

Thanks!