Skip to content
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

Fixes issues #7, #10, #13. Prefix matching related (java equivalent == LookingAt() ) #14

Merged
merged 2 commits into from
Aug 22, 2015

Conversation

loopfz
Copy link
Contributor

@loopfz loopfz commented Aug 20, 2015

In (at least) these 2 places, a regular match/FindIndex was used instead of enforcing matching from the start of the string, like LookingAt() does in the Java implementation.

So for maybeStripNationalPrefixAndCarrierCode, country prefixes (e.g. "0") were matched and truncated at any place in the number, and most cases were saved by the fact that the resulting number would be of an invalid size. But in some situations (like #13), the number legal size would be flexible enough to allow the incorrectly truncated number to go through.

Same thing for parsePrefixAsIdd. Furthermore, line 2553 find[1] was incorrect. You cannot make the assumption that len(find) > 1, so index 1 might be out of range. Java implem used groups(1) because groups(0) has a special meaning (== whole string), but it in fact used the first match, corresponding to find[0] here.

Review on Reviewable

@loopfz
Copy link
Contributor Author

loopfz commented Aug 20, 2015

Unsure why the tests fail now and not before, but the conditions seem wrong:

    if reflect.DeepEqual(getTestNumber("DE_NUMBER"), GetExampleNumber("DE")) {

    if reflect.DeepEqual(
            getTestNumber("DE_NUMBER"), GetExampleNumberForType("DE", FIXED_LINE)) {

Missing a NOT ?

@ttacon
Copy link
Owner

ttacon commented Aug 22, 2015

So those tests were taken from the Java version of libphonenumber so they need to pass in their original form. I don't have a ton of free time at the moment (work and all), but I'll try to get a chance to look deeper into this soon.

@loopfz
Copy link
Contributor Author

loopfz commented Aug 22, 2015

Actually no, if you take a look at https://github.com/googlei18n/libphonenumber/blob/master/java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java#L310 the Java tests assert equality, ie error on non-equality. The tests fix I did in my commit actually detect the issue in #13, and now they pass.

@loopfz
Copy link
Contributor Author

loopfz commented Aug 22, 2015

By the way, I've been reading the Go & Java code side by side for a while now, really huge work you did there, congrats. It's actually very convenient too, the implem being so close to the Java one, it makes it much easier to detect small behavior differences (I have another pending fix I'm working on, some weird behavior on some AR numbers, will PR you later if interested).

ttacon added a commit that referenced this pull request Aug 22, 2015
Fixes issues #7, #10, #13. Prefix matching related (java equivalent == LookingAt() )
@ttacon ttacon merged commit 4124410 into ttacon:master Aug 22, 2015
@ttacon
Copy link
Owner

ttacon commented Aug 22, 2015

LOL. That's what I get for not drinking enough coffee...

@ttacon
Copy link
Owner

ttacon commented Aug 22, 2015

Ya, I would like to make the Go version idiomatic Go, but this library hasn't been on my plate recently so I haven't been able to make the changes I want to. If you've got other PR's fixing issues for this library, please keep them coming lol!

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.

None yet

2 participants