Skip to content

Fixes for PhoneNumber type determination#40

Merged
marmelroy merged 1 commit intomarmelroy:masterfrom
fikus:determine-type
Mar 27, 2016
Merged

Fixes for PhoneNumber type determination#40
marmelroy merged 1 commit intomarmelroy:masterfrom
fikus:determine-type

Conversation

@fikus
Copy link
Copy Markdown
Contributor

@fikus fikus commented Mar 25, 2016

Several fixes for determining the type of a phone number:

  • Determine which metadata record to use based on the number's region,
    not the numeric country code. This fixes using the US metadata for all
    regions using country code 1, for example.
  • Adjust the order we check the type regular expressions. The fixed and
    mobile patterns are the most generic so they should be checked last.
  • Some fixes for regions with leading zeros
  • Fix regex matchesEntirely - add parentheses to the expression

Also update testAllExampleNumbers to check that the expected type is
computed for all example numbers in the metadata.

This fixes #34.

Note: The metadata resources need to be updated to the latest for all tests to
pass. There is an upstream fix for a couple regular expressions containing
an extra space.

Several fixes for determining the type of a phone number:

- Determine which metadata record to use based on the number's region,
  not the numeric country code. This fixes using the US metadata for all
  regions using country code 1, for example.
- Adjust the order we check the type regular expressions. The fixed and
  mobile patterns are the most generic so they should be checked last.
- Some fixes for regions with leading zeros
- Fix regex matchesEntirely - add parentheses to the expression

Also update testAllExampleNumbers to check that the expected type is
computed for all example numbers in the metadata.
@fikus
Copy link
Copy Markdown
Contributor Author

fikus commented Mar 25, 2016

The unit test failures are caused by two bad regular expressions in the metadata for region PM:
"nationalNumberPattern": "41\\d{4} "
"nationalNumberPattern": "55\\d{4} "
(they include a trailing space.)

I don't know exactly how the metadata file is generated so I don't know what exactly the source of this error is.

@marmelroy marmelroy merged commit 5413101 into marmelroy:master Mar 27, 2016
@marmelroy
Copy link
Copy Markdown
Owner

Thanks @fikus. Updating metadata and then we'll see about the pattern issues...

@marmelroy
Copy link
Copy Markdown
Owner

Okay... so the extra space in the PM regular expressions comes directly from Google's libPhoneNumber XML metadata file:
https://github.com/googlei18n/libphonenumber/blob/master/resources/PhoneNumberMetadata.xml
(the file is huge and the offending lines are 19223 and 19227).

@marmelroy
Copy link
Copy Markdown
Owner

Since it definitely looks like a bug - I will fix it manually in PhoneNumberKit's metadata and open an issue on libPhoneNumber.

@marmelroy
Copy link
Copy Markdown
Owner

Thanks a lot for the great work @fikus. These are some serious type determination improvements.

@fikus fikus deleted the determine-type branch March 28, 2016 22:42
@marmelroy
Copy link
Copy Markdown
Owner

Quick update, @fikus - libPhoneNumber acknowledged the PM regex issue and will fix it in an upcoming release.

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.

Determination of PhoneNumber.type is incorrect

2 participants