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

Add "()" when string.match() is executed in js #3769

Merged
merged 4 commits into from
Dec 23, 2024

Conversation

SG-Kang
Copy link
Contributor

@SG-Kang SG-Kang commented Dec 13, 2024

Without "()", when many conditions exist and former one is matched, it returns the wrong value. So adding "()" for the regex

I think it doesn't happen in Java and cpp

[related issue] (raised by myself)
https://issuetracker.google.com/issues/383824582

[java]
https://github.com/google/libphonenumber/blob/master/java/libphonenumber/src/com/google/i18n/phonenumbers/internal/RegexBasedMatcher.java#L49-L56

[cpp]
https://github.com/google/libphonenumber/blob/master/cpp/src/phonenumbers/regex_based_matcher.cc#L51-L65

@SG-Kang SG-Kang requested a review from a team as a code owner December 13, 2024 04:25
@mandlil
Copy link
Contributor

mandlil commented Dec 18, 2024

Hi,

Thank you for contributing to libphonenumber!

We tested with your PR changes, internally we are getting testcase failures for the string type phonenumbers.
Instead of writing changes in matchEntirely function, you can write here(

new RegExp(generalDesc.getNationalNumberPatternOrDefault());
) like new RegExp('^(?:' + generalDesc.getNationalNumberPatternOrDefault() + ')$').

@SG-Kang
Copy link
Contributor Author

SG-Kang commented Dec 19, 2024

Hi,

Thank you for contributing to libphonenumber!

We tested with your PR changes, internally we are getting testcase failures for the string type phonenumbers. Instead of writing changes in matchEntirely function, you can write here(

new RegExp(generalDesc.getNationalNumberPatternOrDefault());

) like new RegExp('^(?:' + generalDesc.getNationalNumberPatternOrDefault() + ')$').

Hello,
Thank you for your thoughtful review.
While testing with the test cases in this repository, I noticed that a Russian uppercase phone number was incorrectly identified as an invalid number. To address this, I added a case-insensitive option to the regular expression, and all test cases passed successfully.
I also saw your PR (#3770). I think addressing the root cause by modifying the "matchesEntirely" function might be necessary since it is called from multiple places. However, if your proposed change adequately resolves the issue, I think your approach is also a good solution.

@SG-Kang SG-Kang closed this Dec 19, 2024
@SG-Kang SG-Kang reopened this Dec 19, 2024
@tvislavski tvislavski merged commit 36db917 into google:master Dec 23, 2024
8 checks passed
@SG-Kang SG-Kang deleted the patch-2 branch December 24, 2024 08:20
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.

3 participants