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

Add libphonenumber 'format' field support. #80

Merged
merged 1 commit into from
Jan 24, 2018

Conversation

dracos
Copy link
Contributor

@dracos dracos commented Oct 1, 2017

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
Copy link

Coverage Status

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

@dracos
Copy link
Contributor Author

dracos commented Oct 1, 2017

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
Copy link
Contributor Author

dracos commented Oct 1, 2017

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
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@DrHyde
Copy link
Owner

DrHyde commented Jan 8, 2018

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.

This stores the format and intlFormat given for each libphonenumber
formatter in the stub country files, and creates a NationallyPreferred
Formatter that will use that data if present.
@dracos
Copy link
Contributor Author

dracos commented Jan 9, 2018

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
Copy link

Coverage Status

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

@DrHyde DrHyde merged commit e9bdfd9 into DrHyde:master Jan 24, 2018
@DrHyde
Copy link
Owner

DrHyde commented Jan 24, 2018

Thanks!

@dracos dracos deleted the 78-store-format-field branch January 25, 2018 10:40
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.

None yet

3 participants