Skip to content

feat: add missing PhoneNumberToCarrierMapper feature that is available in java version#317

Open
wmundev wants to merge 1 commit intotwcclegg:mainfrom
wmundev:feature/carrier-mapper-phone-number
Open

feat: add missing PhoneNumberToCarrierMapper feature that is available in java version#317
wmundev wants to merge 1 commit intotwcclegg:mainfrom
wmundev:feature/carrier-mapper-phone-number

Conversation

@wmundev
Copy link
Copy Markdown
Collaborator

@wmundev wmundev commented May 2, 2026

Changes

  • feat: add missing PhoneNumberToCarrierMapper feature that is available in java version

@wmundev wmundev self-assigned this May 2, 2026
@wmundev wmundev requested a review from Copilot May 2, 2026 14:21
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a C# implementation of carrier-name lookup to bring the library closer to libphonenumber’s Java feature set, reusing the existing prefix-mapping/resource-loading model already used for offline geocoding.

Changes:

  • Add PhoneNumberToCarrierMapper and shared PrefixFileReader support for carrier prefix data from embedded folders or zip resources.
  • Expose PhoneNumberUtil.IsMobileNumberPortableRegion and add unit tests covering production, synthetic, and zipped carrier datasets.
  • Wire carrier resources into project files, CI/build scripts, ignore rules, and README usage/testing documentation.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
csharp/PhoneNumbers/PrefixFileReader.cs New shared loader for prefix-based mapping data used by carrier lookups.
csharp/PhoneNumbers/PhoneNumbers.csproj Embeds carrier resources similarly to geocoding resources.
csharp/PhoneNumbers/PhoneNumberUtil.cs Adds region-level mobile-number-portability lookup API.
csharp/PhoneNumbers/PhoneNumberToCarrierMapper.cs Introduces the public carrier-mapper API and singleton access.
csharp/PhoneNumbers.Test/TestPhoneNumberToCarrierMapper.cs Adds production/test/zipped-path coverage for carrier lookup behavior.
csharp/PhoneNumbers.Test/PhoneNumbers.Test.csproj Embeds test carrier resources and zipped test archive.
appveyor.yml Updates Windows CI packaging steps for carrier resources.
README.md Documents carrier lookup usage and local test zip generation.
.gitignore Ignores generated carrier zip artifacts.
.github/workflows/run_all_tests_and_upload_code_coverage.yml Generates zipped test carrier data for Windows GitHub Actions.
.github/workflows/build_and_run_unit_tests_linux.yml Generates zipped test carrier data for Linux GitHub Actions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread appveyor.yml
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 12 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread README.md
Comment thread csharp/PhoneNumbers.Test/PhoneNumbers.Test.csproj
@wmundev wmundev requested a review from twcclegg May 2, 2026 15:06
@wmundev wmundev marked this pull request as ready for review May 2, 2026 15:06
@wmundev wmundev force-pushed the feature/carrier-mapper-phone-number branch from 46876b4 to aec7dd5 Compare May 3, 2026 12:58
@codecov
Copy link
Copy Markdown

codecov Bot commented May 3, 2026

Codecov Report

❌ Patch coverage is 93.10345% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.85%. Comparing base (06ddfbe) to head (aec7dd5).

Files with missing lines Patch % Lines
csharp/PhoneNumbers/PrefixFileReader.cs 93.61% 2 Missing and 4 partials ⚠️
csharp/PhoneNumbers/PhoneNumberUtil.cs 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #317      +/-   ##
==========================================
+ Coverage   75.27%   75.85%   +0.58%     
==========================================
  Files          34       36       +2     
  Lines        4424     4540     +116     
  Branches     1001     1028      +27     
==========================================
+ Hits         3330     3444     +114     
+ Misses        885      883       -2     
- Partials      209      213       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines +154 to +156
var phonePrefix = countryCallingCode != 1
? countryCallingCode
: (int)(1000 + number.NationalNumber / 10000000);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

C-1. NANPA carrier lookups always return empty in production

This ports Java's 4-digit NANPA split (1650, 1242, …), which is correct only when the data has been pre-split per area code into a binary blob — Java's
build pipeline does that, the C# port doesn't.

The shipped data at resources/carrier/en/1.txt is a single 728-line file with full prefix entries (1242357|BaTelCo, 1242462|BaTelCo, …). The only NANPA
key that ever lands in MappingFileProvider.countryCallingCodes is 1, so Array.BinarySearch(..., 1650) returns negative, GetFileName returns "", and
every +1 … carrier lookup against GetInstance() returns "".

The existing PhoneNumberOfflineGeocoder.cs:418-426 already saw this and commented out the same split, replacing it with var phonePrefix =
countryCallingCode; — relying on AreaCodeMap.Lookup(number) to do the longest-prefix match against the full national number. The geocoder consumes the
same single-file shape from resources/geocoding/en/1.txt.

The test suite hides this:

  • TestGetNameForFixedOrMobileNumber (line 198-204) asserts "" for +1 6502530000 against GetInstance() — passes for the wrong reason.
  • resources/test/carrier/en/1.txt was renamed to 1650.txt to satisfy the test lookup path.

Fix: drop the NANPA split (mirror geocoder line 426): var phonePrefix = countryCallingCode;. Then add a regression test that calls
GetInstance().GetNameForValidNumber for an actual NANPA prefix in the shipped en/1.txt (e.g. Bahamas +1 242 357 xxxx → "BaTelCo").

-Claude

Copy link
Copy Markdown
Owner

@twcclegg twcclegg May 3, 2026

Choose a reason for hiding this comment

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

Claude's recommended fix:

-            // prefix of 4 digits for NANPA instead, e.g. 1650.
-            var phonePrefix = countryCallingCode != 1
-                ? countryCallingCode
-                : (int)(1000 + number.NationalNumber / 10000000);
+            // Java pre-splits NANPA data into 4-digit-keyed blobs (e.g. 1650) at build time. The C#
+            // port ships the source .txt files unsplit, so a single en/1.txt holds every NANPA entry
+            // and AreaCodeMap's longest-prefix lookup matches against the full national number.
+            // Mirrors PhoneNumberOfflineGeocoder, which made the same trade-off.
+            var phonePrefix = countryCallingCode;```

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It seems odd that this was renamed. Claude wants to rename it back.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Furthermore it'll get named by automatically when the next release overwrites the resources folder.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

thanks, no worries, i'll rename it back, i changed it originally because the NANPA test cases weren't covered

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