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 national formatting of numbers. #83

Merged
merged 1 commit into from
Jan 31, 2018

Conversation

dracos
Copy link
Contributor

@dracos dracos commented Oct 1, 2017

This adds a new National formatter that outputs the number in national format, using stored nationalPrefixFormattingRule to work out what to display.

It also adds a format_for_country function that will then return national format if in same country, international otherwise.

As well as standard usage such as being able to show UK numbers as 020...., one important example is Argentina, where the international number +54 9 11 1234-5678 is the national number 011 15-1234-5678 (9 removed, 15 is added between area code and local code).

(This builds upon #80, with one commit on top of that PR.) Fixes #79.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0002%) to 99.967% when pulling b0dbf84 on dracos:79-national-formatting into 850ad31 on DrHyde:master.

@dracos
Copy link
Contributor Author

dracos commented Oct 2, 2017

I've just had a thought - you might well want to provide a country code/+number to the national function, or a different function (similar to how it's provided to new()), and then it would return the national number if it was for that country, or the international number otherwise. Does that sound good?
Update: Have changed how this is done, using an argument format, to allow this.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0002%) to 99.967% when pulling ed2909e on dracos:79-national-formatting into 850ad31 on DrHyde:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0002%) to 99.967% when pulling 5e6d151 on dracos:79-national-formatting into 850ad31 on DrHyde:master.

@dracos
Copy link
Contributor Author

dracos commented Jan 9, 2018

As with the other PR, I've reworked this to add a new formatter (and a helper function). Hope that's more in line with what you're after.

@coveralls
Copy link

coveralls commented Jan 9, 2018

Coverage Status

Coverage increased (+0.0005%) to 99.938% when pulling eaf938c on dracos:79-national-formatting into d281d6b on DrHyde:master.

@DrHyde
Copy link
Owner

DrHyde commented Jan 24, 2018

Thanks, this looks good. A few comments.

Do you think that the NationallyPreferred formatter should be renamed to something like NationallyPreferredIntl? It's a bit of a mouthful but I think it makes the difference between that and National a bit clearer.

Second, I'm not convinced that format_for_country is a good idea to expose to users or even that something like $ar_obj->format_for_country('GB') makes much sense. In general I think a user would care about the national number (so the new National formatter) and the international number (so NationallyPreferred(Intl) or E.123) but they won't care whether the person the international number is relevant to is in GB or RU or SS. And if the method is a good idea then I think it should be on the Number::Phone class, defaulting to returning E.123 if no country-specific formatting information is available.

Third, I wonder if there's some useful overlap with the dial_to method.

None of these are show-stoppers, but I'd like to at least discuss them before I merge and fiddle about with them.

@dracos
Copy link
Contributor Author

dracos commented Jan 24, 2018

Certainly happy to rename the formatter sure, I also couldn't think of anything nicer.

format_for_country, or something like it, is the main thing I actually want and is the reason for most/all of the other PRs :) It is definitely something I as a user want – I am running a website in one country (say the UK), people will be able to provide their mobile phone numbers, those mobile phone numbers won't all be UK ones, but as the site is UK-based I wish to display UK mobile phone numbers in their national UK format (07xxx...) and non-UK mobile phone numbers in their international format (e.g. +1...) – the display is always from the point of view of the site's location. This seems like it would be a common use case for displaying numbers to me. I don't mind where the method is.

dial_to certainly seems related, yes, and something a bit similar could be got by using a UK number as the from for the above – but I wouldn't want it being "clever" and not including the area code if it happened to match whatever I used for the 'from', if you see what I mean.

@DrHyde
Copy link
Owner

DrHyde commented Jan 24, 2018

OK, that makes sense. Can you write a few lines of doco for format_for_country, then I'll get it merged, fiddle about a bit, and get it into the next release.

This adds a new formatter, 'National', that outputs the number in national
format using the stored nationalPrefixFormattingRule to work out what to
display.

It also adds to StubCountry a format_for_country function; supplying a country
code (text or number) will output the number in national format if it is in
that county, international format otherwise.
@dracos
Copy link
Contributor Author

dracos commented Jan 25, 2018

Done, thanks :)

@DrHyde DrHyde merged commit 119e541 into DrHyde:master Jan 31, 2018
@DrHyde
Copy link
Owner

DrHyde commented Jan 31, 2018

Merged. I've done a bit of fiddling as discussed. I also made format_for_country and the two new formatters work even if the object being formatted doesn't have the requisite data, by just temporarily instantiating a stub object if necessary. It's a hack, but at least the functionality is available no matter how objects are created.

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.

National formatting of numbers
3 participants