Skip to content

refactor: match upstream grouping in FormatNumberForMobileDialing - #463

Merged
twcclegg merged 4 commits into
mainfrom
fix/mobile-dialing-non-geo-parity
Sep 6, 2026
Merged

refactor: match upstream grouping in FormatNumberForMobileDialing#463
twcclegg merged 4 commits into
mainfrom
fix/mobile-dialing-non-geo-parity

Conversation

@twcclegg

@twcclegg twcclegg commented Sep 5, 2026

Copy link
Copy Markdown
Owner

What this is — and what it is not

No behavior change. This is a parity/robustness refactor, not a bug fix.

The divergence

Upstream Java (PhoneNumberUtil.java:1535-1551) guards the whole disjunction:

if ((regionCode.equals(REGION_CODE_FOR_NON_GEO_ENTITY)
     || ((regionCode.equals("MX") || regionCode.equals("CL")
         || regionCode.equals("UZ")) && isFixedLineOrMobile))
    && canBeInternationallyDialled(numberNoExt)) {

The port dropped the outer parentheses, so && binds tighter and reads as NON_GEO || (MX/CL/UZ && isFixedLineOrMobile && CanBeInternationallyDialled) — the dialling check applies only to the MX/CL/UZ arm.

Why it is not observable

CanBeInternationallyDialled returns true for every non-geographical number by construction, before it consults any metadata:

var regionCode = GetRegionCodeForNumber(number);
if (!IsValidRegionCode(regionCode))
    // Note numbers belonging to non-geographical entities (e.g. +800 numbers) are always
    // internationally diallable, and will be caught here.
    return true;

So in the only case where the grouping matters — regionCode == "001" — the guard is unconditionally true and both shapes reduce to the same thing. For MX/CL/UZ the two forms are already identical. The expressions agree on every reachable input; this is a structural invariant (any country calling code whose region list is ["001"]), not a fragile coincidence.

Tests

This PR adds UN001/InternationalTollFree assertions (the non-geo arm) and MXMobile1/MXNumber1 assertions (the MX arm) to TestFormatNumberForMobileDialing, closing a real gap — neither branch had any prior coverage.

These tests do not, and cannot, distinguish the old grouping from the new one — per the equivalence proven above, whenever the non-geo/A branch is true, CanBeInternationallyDialled is provably always true too, so no assertion exercising that branch can tell the two shapes apart. They're legitimate incidental coverage of previously-untested formatting paths, not regression tests for this specific change. The only argument for the fix itself is the algebraic one above.

Why land it anyway

The equivalence is a coincidence of an invariant living in a different method. If CanBeInternationallyDialled ever starts consulting non-geographical metadata — which upstream could do without it looking like a breaking change here — this port would silently diverge on formatting, with no test able to catch it. The port's value is being a faithful mirror of upstream, and matching the grouping costs two characters.

Verification

  • dotnet build csharp/PhoneNumbers.slnx -p:TargetFrameworks=net10.0 — 0 warnings, 0 errors.
  • dotnet test csharp/PhoneNumbers.slnx -p:TargetFrameworks=net10.0 — 440 + 51 passed, 0 failed, 0 skipped (unchanged counts, as expected for a behavior-preserving change).

Upstream Java guards the whole non-geographical/MX/CL/UZ disjunction with
canBeInternationallyDialled:

    if ((regionCode.equals(REGION_CODE_FOR_NON_GEO_ENTITY)
         || ((MX || CL || UZ) && isFixedLineOrMobile))
        && canBeInternationallyDialled(numberNoExt))

The port dropped the outer parentheses, so && binds tighter and the check
applies only to the MX/CL/UZ arm.

That is not observable today: CanBeInternationallyDialled returns true for
every non-geographical number by construction, before it ever consults
metadata, so the two shapes agree on all reachable inputs. No test can tell
them apart and none is added. The equivalence rests on that invariant
holding inside a different method, though, and the port's value is being a
faithful mirror of upstream - so restore the grouping rather than leave a
divergence that only stays harmless by coincidence.
@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.64%. Comparing base (8ad7fa2) to head (f017f09).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #463      +/-   ##
==========================================
+ Coverage   87.51%   87.64%   +0.12%     
==========================================
  Files          43       43              
  Lines        3886     3885       -1     
  Branches      991      991              
==========================================
+ Hits         3401     3405       +4     
+ Misses        280      277       -3     
+ Partials      205      203       -2     

☔ View full report in Codecov by Harness.
📢 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.

twcclegg and others added 3 commits September 5, 2026 00:47
Every existing FormatNumberForMobileDialing case for InternationalTollFree
dials from JP, so regionCallingFrom never equals the number's own region
and the non-geographical arm of the regionCallingFrom == regionCode branch
- the one whose grouping this branch restores - was never executed.

Add the 001-to-001 case in both formatting modes.
The MX/CL/UZ half of the condition had no test at all: no case dialled a
Mexican number from MX, so short-circuit evaluation never reached that
operand. Assert the documented behaviour - MX fixed line and mobile numbers
are formatted internationally even when dialled within MX, because a
national-format call needs a carrier code that depends on both parties'
local area.
An editor flipped the whole file from CRLF to LF while adding the
UN001/MX test cases, turning a ~20-line addition into a ~5,800-line
diff that buries the real change and leaves this one file on a
different line-ending convention than the rest of the C# codebase.

No content change - only line endings.
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.

2 participants