Skip to content

fix: narrow TryParse/TryParseValid catch to NumberParseException - #401

Closed
twcclegg wants to merge 1 commit into
mainfrom
fix/extensions-tryparse-narrow-catch
Closed

fix: narrow TryParse/TryParseValid catch to NumberParseException#401
twcclegg wants to merge 1 commit into
mainfrom
fix/extensions-tryparse-narrow-catch

Conversation

@twcclegg

Copy link
Copy Markdown
Owner

Summary

  • PhoneNumbers.Extensions.PhoneNumber.TryParse/TryParseValid used a bare catch { }, silently swallowing any exception (not just parse failures) and masking bugs.
  • Narrows both to catch (NumberParseException), matching the exception type PhoneNumberUtil.Parse actually throws.

Test plan

  • dotnet build csharp --no-restore — clean, 0 warnings/errors
  • dotnet test csharp/PhoneNumbers.slnx — 864 tests passed (net8.0 + net10.0), including the existing TryParse("1235557704", "ZX", ...) case that already exercises the narrowed exception path

The bare catch in PhoneNumbers.Extensions.PhoneNumber swallowed every
exception, not just parse failures, silently masking bugs.
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.26%. Comparing base (a40ff86) to head (a69a16b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #401   +/-   ##
=======================================
  Coverage   77.26%   77.26%           
=======================================
  Files          39       39           
  Lines        4548     4548           
  Branches     1129     1129           
=======================================
  Hits         3514     3514           
  Misses        783      783           
  Partials      251      251           

☔ 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

Copy link
Copy Markdown
Owner Author

Review confirmed this narrowing is correct and exposed (rather than caused) a pre-existing bug in PhoneNumberUtil.Parse — fixed in #406. Recommend merging #406 alongside this one.

@twcclegg

Copy link
Copy Markdown
Owner Author

Superseded by #405.

@twcclegg twcclegg closed this Aug 25, 2026
@twcclegg
twcclegg deleted the fix/extensions-tryparse-narrow-catch branch August 25, 2026 00:01
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.

1 participant