search

nyaruka / phonenumbers

GoLang port of Google's libphonenumber library
MIT License
1.25k stars 148 forks source link

Update `GetLengthOfGeographicalAreaCode` to match recent libphonenumber changes #176

Closed rowanseymour closed 4 months ago

rowanseymour commented 4 months ago

Addresses https://github.com/nyaruka/phonenumbers/issues/173

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 46.81%. Comparing base (2aea864) to head (bf96e96).

Files Patch % Lines
phonenumbers.go 90.90% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #176 +/- ## ========================================== + Coverage 46.75% 46.81% +0.06% ========================================== Files 11 11 Lines 3788 3798 +10 ========================================== + Hits 1771 1778 +7 - Misses 1867 1869 +2 - Partials 150 151 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alicebob commented 4 months ago

Hi,

are you sure this is correct? Before this update +5214424123123 rendered as +524424123123 (the first "1" is gone). "+524424123123" also renders to that version (so it stays the same). The "521" prefix is an old one which is not in use anymore.

Random reference from the Interwebs:

https://www.jaxcom.net/blog/important-changes-to-mexico-521-dialing-pattern/ All calls to Mexico mobile phones: Will no longer need the prefix of ‘521’ followed by the phone number. Must strictly be dialed as ’52’ followed by the phone number.

edit: actually was probably changed in #175

rowanseymour commented 4 months ago

Hi @alicebob if you think there's a parsing problem please confirm the problem doesn't also exist in https://github.com/googlei18n/libphonenumber

alicebob commented 4 months ago

Even worse, that one rejects it with the 1:

https://libphonenumber.appspot.com/phonenumberparser?number=%2B524424123123 E164 format: +524424123123

https://libphonenumber.appspot.com/phonenumberparser?number=%2B5214424123123 E164 format: invalid

(This version of nyaruka accepts the one, and an older one dropped it)

rowanseymour commented 4 months ago

Please report that then as an issue on that library as we are just a port of that and use their metadata

alicebob commented 4 months ago

On Tue, Jul 23, 2024 at 07:44:04AM -0700, Rowan Seymour wrote:

Please report that then as an issue on that library as we are just a port of that and use their metadata

But nyaruka should also reject it, currently it doesn't.

rowanseymour commented 4 months ago

I agree but we are using their metadata, so it needs to be fixed there first

alicebob commented 4 months ago

thanks @rowanseymour . I made a separate issue, instead of talking in a closed PR.