search

nyaruka / phonenumbers

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

Fix regex func #71

Closed cristaloleg closed 4 years ago

cristaloleg commented 4 years ago

Hi, we've encountered a problem with a phone from Indonesia (62816640000 to be exact) when we tried to get a national number.

After verification this phone with a JS version (can be checked here, on the latest master https://rawgit.com/googlei18n/libphonenumber/master/javascript/i18n/phonenumbers/demo-compiled.html) we found that Go versions return national number with a country code (when it shouldn't).

phone: 62816640000
countyCode: ID

national number:
  Go: 62816640000
  JS: 816640000

The problem lies in a regular expression, 'cause it used slightly in a different way compared to JS. As you can see here google/libphonenumber on the current master (v8.12.13) it checks that some group was found AND it equals to the whole string.

While in Go it's a simple match nyaruka/phonenumbers on the current master (v1.0.59 (and Go docs confirms this https://pkg.go.dev/regexp#Regexp.MatchString).

After fixing this regex step, we've encountered a failing test (TestFormatInOriginalFormat) for 49987654321 DE and after another comparison to the JS we found another difference, instead of 49 9876 54321 should be 4998 7654321.

Thank you. CC: @papisz

nicpottier commented 4 years ago

Woot, awesome PR, thank you for this.

Do you guys use this library regularly? Interested in being a maintainer?

cristaloleg commented 4 years ago

First, thank you for the merge, much appreciate.

We've started using your (awesome by the way!) library less than a month ago and it solves our problems perfectly.

If you've something on your mind (roadmap, features etc), feel free to ping me (see profile for any communication channels).

cristaloleg commented 4 years ago

And one more: any plans to release v1.0.60 ?

nicpottier commented 4 years ago

Ya, I'm revving the metadata and will cut a release shortly.

I'll add you as a contributor. There isn't a roadmap, but there's a bit of constant care needed in revving the metadata, addressing translation bugs as you found etc.. and having someone else on that front is always good!

nicpottier commented 3 years ago

So I think this fix is wrong and is causing problems, see: https://github.com/nyaruka/phonenumbers/issues/80

The fix of changing to FindAllString isn't actually right, the full pattern is matched by the creation of patP above which adds start ^ and end $ anchors to the regex.

I'm not sure about the ID problem you originally encountered though @cristaloleg, trying to dig into that.

Trying to figure that all out here: https://github.com/nyaruka/phonenumbers/pull/83