-
Notifications
You must be signed in to change notification settings - Fork 148
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
Update GetLengthOfGeographicalAreaCode
to match recent libphonenumber changes
#176
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
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:
edit: actually was probably changed in #175 |
Hi @alicebob if you think there's a parsing problem please confirm the problem doesn't also exist in https://github.com/googlei18n/libphonenumber |
Even worse, that one rejects it with the 1: https://libphonenumber.appspot.com/phonenumberparser?number=%2B524424123123 https://libphonenumber.appspot.com/phonenumberparser?number=%2B5214424123123 (This version of nyaruka accepts the one, and an older one dropped it) |
Please report that then as an issue on that library as we are just a port of that and use their metadata |
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.
|
I agree but we are using their metadata, so it needs to be fixed there first |
thanks @rowanseymour . I made a separate issue, instead of talking in a closed PR. |
Addresses #173