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

Rename NonGeographicalRegion to NonGeographicalEntity. #3627

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

KarolJakubKrawiec
Copy link

@KarolJakubKrawiec KarolJakubKrawiec commented Sep 18, 2024

This is to be consistent with the naming of non-geographical entities, since region and non-geographical can be contradictory.

Context: b/206752240

@KarolJakubKrawiec KarolJakubKrawiec requested a review from a team as a code owner September 18, 2024 12:16
Copy link
Member

@tvislavski tvislavski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also update https://github.com/google/libphonenumber/blob/master/pending_code_changes.txt with the explanation of this change? This will be included in the release notes then

@KarolJakubKrawiec KarolJakubKrawiec force-pushed the rename-nongeographicalregion branch from acb49f2 to e49a7ac Compare September 19, 2024 07:21
tvislavski
tvislavski previously approved these changes Oct 2, 2024
This is to be consistent with the naming of non-geographical entities, since region and non-geographical can be contradictory.
Added ( - Renamed NonGeographicalRegion to NonGeographicalEntity.
 - Created a new interface called NonGeographicalEntityMetadataSourceV2) into the file.
Changed @deprecated use the following... to just @deprecated Use... to make every @deprecated comment the same.
I renamed the NonGeographicalRegion variables and methodes to NonGeographicalEntity because we want to keep the naming the same so its easier to read. This CL is for the C++ version.
@KarolJakubKrawiec KarolJakubKrawiec force-pushed the rename-nongeographicalregion branch from ae42ce6 to 32e0765 Compare November 28, 2024 12:56
Comment on lines 865 to 866
const i18n::phonenumbers::PhoneMetadata* GetMetadataForNonGeographicalRegion(
int country_calling_code) const;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this since it is a private function.

Comment on lines 67 to +70
const PhoneMetadata* GetMetadataForNonGeographicalRegion(
int country_code) const {
return phone_util_.GetMetadataForNonGeographicalRegion(country_code);
return phone_util_.GetMetadataForNonGeographicalEntity(country_code);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also remove this because you already renamed the calls to this function.

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