Added PhoneNumberRange and numerical operators on PhoneNumber #2
Conversation
…iguous phonenumbers. Added methods to PhoneNumber to support comparing phone numbers numerically which is required when managing ranges of phone numbers.
…eption. This should make it easier to track the source of an error when processing lists of phone numbers. Minor grammatical improvements to exception descriptions.
|
Added exmaples. |
|
Nice, thank you. Could you give an use case for this ? An use case for knowing that a phone number is greater than another one or sequential. It might be obvious, like sorting, just to understand |
|
We manage large numbers of phone numbers.
We generally hold these and a range -
03 8302 8100 - 03 8320 8199
We then allocate numbers from these ranges which requires us to split them
into smaller ranges.
To do this we need to select a phone number from the range then find the
next number in the range which will be the start of the new range.
e.g.
Existing range
03 8302 8100 - 03 8320 8199
Allocate
03 8302 8100
New ranges are:
03 8302 8100 - 03 8320 8100
03 8302 8101 - 03 8320 8199
We use numeric addition (03 8320 8100 + 1) to derive 03 8302 8101 as the
start of the new range.
We also perform audits on ranges of numbers to ensure that they don't
overlap so the comparison operators <, > ... are required.
S. Brett Sutton
Noojee Contact Solutions
03 8320 8100
…On Tue, 7 Sept 2021 at 10:17, cedvdb ***@***.***> wrote:
Nice. Could you give your use case for this ? An use case for knowing that
a phone number is greater than another one.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OAPFHHQKXHHTBNQEJLUAVK25ANCNFSM5DRHY4AA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
| const PhoneNumberRange( | ||
| this.start, | ||
| this.end, | ||
| ); |
There was a problem hiding this comment.
I'm wondering if we should support a backward range, if the starting range is greater than the end, maybe by swapping them if that's the case.
There was a problem hiding this comment.
yes we only ever use forward ranges.
I think it might be safer if we throw if the try to supply a forward range.
If we just swap we might cause confusion when the range is printed.
|
|
||
| var first = BigInt.parse(_number(start)); | ||
| var last = BigInt.parse(_number(end)); | ||
|
|
There was a problem hiding this comment.
I'm having some trouble wrapping my head around some things here as it seems you have a future vision.
- You used
_numberwhile there is aninternationalproperty already. Since you added the leading zero check I'm wondering if that's intentional and you expect the _number property to change. - Your comment above the leading zero check is about national number but here you are using the international version, which (currently) would only make sens for checking for international prefix starting with 0, (eg 00).
Unless I'm mistaking, something seems off ? Maybe the idea is to expand ranges of phone numbers without their dial code and with their national prefix, but that's not what's happening here.
There was a problem hiding this comment.
Going forward I would like to see support for national and local phone formats. The international format isn't particularly friendly for end users. National and local formats are much easier to read.
In anticipation of support for local and national codes I want to support leading zeros.
The international method includes a '+' symbol which is why I strip it off and relates to leading zeros.
I guess the questions here is would we ever support a local number without knowing the dial code.
If we won't then there is no point supporting leading zeros.
There was a problem hiding this comment.
I would like to see support for national and local phone formats.
That's certainly possible. In another of your comment you mentioned formatting to its local format. I assumed a property like .local on the phone number class is what you had in mind. IE local would return the national number in its local form without the dial code
However, your method expand range would return PhoneNumbers right ? You can then use the .local property on those. The leading zero check would still be done on the _number which is starting with a dial code.
So, as far as I understand, it does not makes sens to use the leading zero check here as it is only confusing as it is currently misapplied. If the local format is added, it would still not make sens as it does not entail that the _number property will change. That's where I don't follow why it is used at all.
the questions here is would we ever support a local number without knowing the dial code.
By " without knowing the dial code " I assume that you mean that the library does not try to figure out where a phone number originates from (in comparison, parseRaw does). If that's the case, the library is not aware of the phone metadata, which means you cannot transform, validate or learn anything about the phone number. It is just a string. The only thing that could be used is the range but at that point it sounds more like string manipulation than phone number parsing. I can see an use case maybe though.
The international method includes a '+' symbol which is why I strip it off and relates to leading zeros.
Does it matter ? BigInt.parse('+45') == BigInt.parse('45')
As you can see I'm a bit reluctant to add code that is currently unnecessary, for the sake of readability. I'd prefer adding those once it is necessary.
|
Thank you, that looks great. I'm just hoping this is not used for phone marketing Spam ahah |
|
And no, not used for spam. We sell phone numbers to customers. We don't make phone calls. |
|
Firstly I'm concerned about the terminology bring used.
Phone numbers in the United States typically consist of 11 digits — the
1-digit country code, a *3-digit area code and a 7-digit telephone number*.
The 7-digit telephone number is further comprised of a 3-digit central
office or exchange code and a 4-digit subscriber number.
What you refer to as the dial code i thick is actually the country code.
My understanding is the dial code is the prefix used in place of the +. Eg
in au this is 0011 whilst the country code is 64.
Dial codes aren't actual country specific but rather telco specific.
I don't think you can know if a no is a national or local no without
knowning the country code.
I have to rethink my normal approach as normally I only deal with au no.s.
We could deal with this by creating explicit constructors.
Phone number.fromNational. fromLocal.
We could always force a country code as that tells us how to format the no.
So know we have phone numbers not strings and we are back to dealing with
leading zeros.
…On Tue, 7 Sep 2021, 8:44 pm cedvdb, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/src/models/phone_number_range.dart
<#2 (comment)>
:
> +
+ var interval = bigInterval.abs().toInt() + 1;
+
+ return interval;
+ }
+
+ String _number(PhoneNumber phone) => '${phone.dialCode}${phone.nsn}';
+
+ /// Returns a list of [PhoneNumber]s in the range
+ /// [this] to [endOfRange] inclusive.
+ List<PhoneNumber> expandRange() {
+ var startZeros = _getLeadingZeros(start);
+
+ var first = BigInt.parse(_number(start));
+ var last = BigInt.parse(_number(end));
+
I would like to see support for national and local phone formats.
That's certainly possible. In another of your comment you mentioned
formatting to its local format. I assumed a property like .local on the
phone number class is what you had in mind.
However, your method expand range would return PhoneNumbers right ? You
can then use the .local property on those. The leading zero check would
still be done on the _number which is starting with a dial code.
So, as far as I understand, it does not makes sens to use the leading zero
check here as it is only confusing as it is currently misapplied.
the questions here is would we ever support a local number without knowing
the dial code.
By " without knowing the dial code " I assume that you mean that the
library does not try to figure out where a phone number originates from (in
comparison, parseRaw does). If that's the case, the library is not aware
of the phone metadata, which means you cannot transform, validate or learn
anything about the phone number. It is just a string. The only thing that
could be used is the range but at that point it sounds more like string
manipulation than phone number parsing.
The international method includes a '+' symbol which is why I strip it off
and relates to leading zeros.
Does it matter ? BigInt.parse('+45') == BigInt.parse('45')
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OGISN3R3WEC5DBWYJTUAXUI5ANCNFSM5DRHY4AA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
We are not using the same terminology indeed, so I'll be a bit clearer: phone_numbers_parser That property should be renamed to avoid confusion as it seems to be misleading. I will get on it asap. |
|
I'm having mixed results in my google searches. I initially used the name "dial Code" instead of country code because I found that "country code" was easily confused with the iso code. The wikipedia article on country code states
So it seems like "country code" can sometimes mean iso code (alpha-2 above) and other times the digits country calling code. Therefor might still be confused with the iso code. Dial code is also used differently in different context, sometimes meaning the country code and sometimes meaning the country code + the area like here https://countrycode.org/australia. Which you said is actually telco specific. I'm guessing "countryCallingCode" might be a better name for this to alleviate some of the possible confusions, alternatively "countryDialCode". I'm hoping someone like yourself who works in the field could help me pick the better name so I don't make the same mistake twice. |
|
The terminology I would suggest is:
dialCode - the dial prefix such as 0011
countryCode - the numeric country code e.g. 64
isoCountryCode - the alphabetical country code e.g. au.
Alternatives to dialcode:
dialPrefix
internationalPrefix
internationalCode
exitCode
internationalExitCode
internationalAccessCode
For a breaking change in the api it might be better to use one of the
alternatives to dialcode so users understand there has been a change.
Of the above I prefer internationalPrefix as it is the most evocative of
the alternatives.
It might also be worth talking about 'area code'. I see descriptions such
as:
0011 + Country Code + (Area Code + Telephone Number)
Should the api split out the area code?
It feels like it would be useful as it makes it easy to extract a local
number from a national number if we know what the area codes look like.
Some more descriptions I've found:
replace the + sign with 0011 followed by 1 (US country code) and then the
US area code and local number.
Ring Central - large international provider
1. An exit code – otherwise known as the dial out code, an international
call prefix, or IDD prefix (international direct dialing) which is used to
dial out from your country,
2. A dial in code – or the country code or the country’s international
number, which designated the country to which your call will be directed,
and,
3. The phone number including the geographic area code or city code
designating the place you are calling. When dialing internationally, the
first 0 of the area code is dropped.
So the term they use is 'dial in code' for country code. I wonder if this
is the source of confusion.
Ring Central also uses the term 'dial out code' and exit
code interchangeably.
So from the above my thoughts are the PhoneNumber class should have:
internationalExitCode (which could be a + sign) - e.g. 0011
countryCode - e.g. 64
isoCountryCode - e.g. AU
areaCode - e.g. 03
localNumber - 8320 8100
This arrangement appears to provide the most flexibility and a set of
properties that are evocative of what information they provide.
S. Brett Sutton
Noojee Contact Solutions
03 8320 8100
…On Wed, 8 Sept 2021 at 10:04, cedvdb ***@***.***> wrote:
I'm having mixed results in my google searches. I initially used the name
"dial Code" instead of country code because I found that "country code" was
easily confused with the iso code.
The wikipedia article on country code states
Several different systems have been developed to do this. The term country
code frequently refers to ISO 3166-1 alpha-2 or international dialing
codes, the E.164 country calling codes.
So it seems like "country code" means different things depending on the
context and can mean iso code in some context (alpha-2 above).
However it seems like dial code is also used differently in different
context, sometimes meaning the country code and sometimes meaning the
country code + the area like here https://countrycode.org/australia
I'm guessing "countryCallingCode" might be a better name for this to
alleviate some of the possible confusions.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OFZWH6IJN7FWKXMZDLUA2SADANCNFSM5DRHY4AA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
I added a new branch The other properties did not change, as they align with google metadatas properties name. So there is
As for the area code it is not yet added. |
|
I will try to find the time tonight.
I'm not in love with the name countryCallingCode it's too close to
countryCode.
countryExitCode might be a better name.
If we are going to make breaking changes then I suggest that we consider
the other properties and breakup of the data I outlined.
areaCode/phoneNo will be a significant improvement over nsn.
Bbeing able to display national vs local numbers is important.
Users prefer to see a local number when dialing in their own local area and
the current class doesn't allow this.
…On Thu, 9 Sep 2021, 7:31 am cedvdb, ***@***.***> wrote:
I added a new branch country_code where dial code was renamed to
countryCallingCode. I had to change a lot of instance of the word dial
code happens. Would it be possible to rebase on that one ? If it's too
annoying I can help. Since your changes are mainly on an external file I
don't think it will though.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OGOYGVQWIAGWBRTG2LUA7I2TANCNFSM5DRHY4AA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Totally. I'm not sure this breaking change is required. However it seems like the dialCode property was confusing. If you search for "country code australia" the first result that pops up in google is "dialing code": 61. Honnestly if we can get away with a breaking change I think it would be better, but not at the expense of being semantically wrong.
I'm confused. Those two are the same now. Both are 61 for australia. So I'm not sure I understand what the complain is, I could have used "country code" I just believe it's too ambiguous with the alpha-2 code, adding calling in the middle makes it more obvious those are digits. I'm using google metadata in the background and this library is based on the implementations found in libphonenumber and phoneNumberKit for ios. Those two libraries use the following properties names:
https://github.com/google/libphonenumber/blob/master/resources/PhoneNumberMetadata.xml I would like to stay as close as possible to them when possible except if the property names are confusing. I believe They use the nsn for nationalSignificantNumbers, internally in their implementation. Currently the As for the area code this would be a great addition. This data is not in the metadata from google though. However it seems like this data is available here: https://github.com/google/libphonenumber/tree/master/resources/geocoding/en so that's somewhere I could add the metadata for area code from. I would like to keep the build size not too large though, so how it will be added remains to be seen. Maybe a separate Geocoder class but it won't be part of the main parsing process as it is a niche requirement. So we would have:
And a GeoCoder that further divide the remaining into:
As for |
|
Hello, I took the liberty to pull the changes and fix my comments and merge it into dev as this was stalling a bit (my fault). If you see anything that you'd like let me know. I did not change anything beside my comments and added a section in the readme.
Phone numbers are not my area of expertise to begin with, so I'm also learning while building the library, thanks a lot for the directions. |
I have the requirement to manage large no.s of phone number ranges.
This PR includes a new class PhoneNumberRange which can store a contiguous range of phone numbers.
New operators have been added to the PhoneNumber class to facilitate implementation of a ranges and numeric comparison of phone numbers.
Small improvements to the description of exceptions to make locating the data that caused the problem easier.
Added unit tests for PhoneNumberRange and the new operators in PhoneNumber.
All unit tests are passing.