Create new class ParseWithOptions by KarolJakubKrawiec · Pull Request #3730 · google/libphonenumber

KarolJakubKrawiec · 2024-11-27T13:04:04Z

Created a new class called ParseWithOptions that should allow for easier use of different methods such as keepRawInput or defaultRegion.

I have added the new parsing methode ParsingOptions to the OpenSource version.

There is a new class called PrasingOptions. With it we can replace old parsing methodes such as keepRawInput or defaultRegion.

I have added the new parsing methode ParsingOptions to the OpenSource version.

There is a new class called PrasingOptions. With it we can replace old parsing methodes such as keepRawInput or defaultRegion.

We had to switch up the deprecated test cases of parseAndKeepRawInput and replaced them with parseWithOptions. It includes the phonenumber that has to be parsed and the options such as if the raw input should be kept or what the default regioncode is.

silvio2402

Good! Please also merge the changes from the base branch.

silvio2402 · 2024-12-11T16:06:31Z

java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java

    // National prefix attached.
    assertEquals(NZ_NUMBER, phoneUtil.parse("033316005", RegionCode.NZ));
-    // Some fields are not filled in by parse, but only by parseAndKeepRawInput.
+    // Some fields are not filled in by parse, but only by parseWithOptions.


Can you add " with the keepRawInput option set."?

silvio2402 · 2024-12-11T16:10:23Z

java/libphonenumber/src/com/google/i18n/phonenumbers/ParsingOptions.java

+  /**
+   * Returns the region we are expecting the number to be from. This is ignored if the number being
+   * parsed is written in international format. In case of national format, the country_code will be
+   * set to the one of this default region. If the number is guaranteed to start with a '+' followed
+   * by the country calling code, then RegionCode.ZZ or null can be supplied.
+   */
+


Can you move the javadoc above getDefaultRegion?

I rewrote some comments because they didnt make sense and were misplaced

In this commit we added the ParsingOptions from the java version which will make parsing in different ways easier

We created a new function that allows the user for customized parsing. You can choose based on what you want the numbers to be parsed.

silvio2402

Good job!

silvio2402 · 2025-01-29T15:53:35Z

cpp/src/phonenumbers/geocoding/geocoding_data.cc

Please remove this auto-generated file.

silvio2402 · 2025-01-29T16:05:18Z

cpp/src/phonenumbers/parsingoptions.h

+// Options for parsing a phone number. To be used with the ParseWithOptions
+// method.
+// Example:
+//   ParsingOptions().SetDefaultRegion(RegionCode::US()).SetKeepRawInput(true);


Please update the example to use a string for the region code.

silvio2402 · 2025-01-29T16:09:47Z

cpp/src/phonenumbers/parsingoptions.h

+  // number being parsed is written in international format. In case of national
+  // format, the country_code will be set to the one of this default region. If
+  // the number is guaranteed to start with a '+' followed by the country
+  // calling code, then RegionCode.ZZ or null can be supplied.


Suggested change

// calling code, then RegionCode.ZZ or null can be supplied.

// calling code, then "ZZ" or null can be supplied.

silvio2402 · 2025-01-29T16:10:49Z

cpp/src/phonenumbers/parsingoptions.h

+}  // namespace phonenumbers
+}  // namespace i18n
+
+#endif  // I1


Suggested change

#endif // I1

#endif // I18N_PHONENUMBERS_PARSINGOPTIONS_H_

silvio2402 · 2025-01-29T16:11:25Z

cpp/src/phonenumbers/phonemetadata.pb.cc

auto-generated file

silvio2402 · 2025-01-29T16:42:26Z

cpp/src/phonenumbers/parsingoptions.h

@@ -0,0 +1,42 @@
+#ifndef I18N_PHONENUMBERS_PARSINGOPTIONS_H_


Please add the license header.

silvio2402 · 2025-01-29T16:48:53Z

java/libphonenumber/src/com/google/i18n/phonenumbers/ParsingOptions.java

+  /**
+   * Returns the region we are expecting the number to be from. This is ignored if the number being
+   * parsed is written in international format. In case of national format, the country_code will be
+   * set to the one of this default region. If the number is guaranteed to start with a '+' followed
+   * by the country calling code, then RegionCode.ZZ or null can be supplied.
+   */
+  private boolean hasDefaultRegion;
+  private String defaultRegion_ = null;
+  public boolean hasDefaultRegion() { return hasDefaultRegion; }
+  public String getDefaultRegion() { return defaultRegion_; }
+  public ParsingOptions setDefaultRegion(String value) {
+    hasDefaultRegion = (value != null);
+    defaultRegion_ = value;
+    return this;
+  }


Please rephrase the comment to not say "returns" and move the comment above the field that holds the value (in this case defaultRegion_). Then add JavaDoc comments to each public function. The function comments should all have a link to the field from before.

silvio2402 · 2025-01-29T16:50:08Z

java/libphonenumber/src/com/google/i18n/phonenumbers/ParsingOptions.java

+  /**
+   * Returns whether the raw input should be kept in the PhoneNumber object. If true, the raw_input
+   * field and country_code_source fields will be populated.
+   */
+  private boolean hasKeepRawInput;
+  private boolean keepRawInput_ = false;
+  public boolean hasKeepRawInput() { return hasKeepRawInput; }
+  public boolean isKeepRawInput() { return keepRawInput_; }
+  public ParsingOptions setKeepRawInput(boolean value) {
+    if(value == false) {
+
+    }
+    hasKeepRawInput = true;
+    keepRawInput_ = value;
+    return this;
+  }


See above comment regarding the comments.

silvio2402 · 2025-01-29T16:50:46Z

java/libphonenumber/src/com/google/i18n/phonenumbers/ParsingOptions.java

+    if(value == false) {
+
+    }


Please remove this unneeded if-statement.

silvio2402 · 2025-01-29T16:54:44Z

java/libphonenumber/src/com/google/i18n/phonenumbers/ParsingOptions.java

+  public ParsingOptions withDefaultRegion(String regionCode) {
+    return setDefaultRegion(regionCode);
+  }


Since we aren't using a Builder here creating a ParsingOptions object is pretty short. Hence I'd say we don't need this function here.

Added java doc comments and deleted unnecessary functions

KarolJakubKrawiec requested a review from a team as a code owner November 27, 2024 13:04

silvio2402 self-requested a review November 27, 2024 13:06

KarolJakubKrawiec force-pushed the parseWithOptions branch from a09decd to 251e95e Compare November 28, 2024 13:00

KarolJakubKrawiec added 4 commits November 28, 2024 13:07

Add parsingOptions to OpenSource

ed15e47

I have added the new parsing methode ParsingOptions to the OpenSource version.

Create new class ParsingOptions

f5c2748

There is a new class called PrasingOptions. With it we can replace old parsing methodes such as keepRawInput or defaultRegion.

Add parsingOptions to OpenSource

809f889

I have added the new parsing methode ParsingOptions to the OpenSource version.

Create new class ParsingOptions

45cb5e4

There is a new class called PrasingOptions. With it we can replace old parsing methodes such as keepRawInput or defaultRegion.

KarolJakubKrawiec force-pushed the parseWithOptions branch from 251e95e to 45cb5e4 Compare November 28, 2024 13:08

silvio2402 reviewed Dec 11, 2024

View reviewed changes

KarolJakubKrawiec added 6 commits December 12, 2024 09:41

Rewrite comments

5155f58

I rewrote some comments because they didnt make sense and were misplaced

Create ParsingOptions for cpp

9d7975a

In this commit we added the ParsingOptions from the java version which will make parsing in different ways easier

Updated (NON FUNCTIONAL) Version of ParseWithOptions

1a8629e

ParsingOptions update (non functional)

ddcfb51

ParseWithOptions Opensource CPP Version

2b6d054

We created a new function that allows the user for customized parsing. You can choose based on what you want the numbers to be parsed.

Merge branch 'google:master' into parseWithOptions

65f0f44

silvio2402 suggested changes Jan 29, 2025

View reviewed changes

KarolJakubKrawiec added 2 commits February 6, 2025 09:35

Merge branch 'google:master' into parseWithOptions

35f998d

Solved mistakes for ParseWithOptions

d9d2ef0

Added java doc comments and deleted unnecessary functions

	// calling code, then RegionCode.ZZ or null can be supplied.
	// calling code, then "ZZ" or null can be supplied.

Conversation

KarolJakubKrawiec commented Nov 27, 2024

Uh oh!

silvio2402 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvio2402 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants