refactor: simplify IsLatinLetter range test, document broad catch - #461
Merged
Conversation
Two of the four note-severity CodeQL maintainability alerts in this group were worth acting on; the other two were not, and are left alone deliberately. PhoneNumberMatcher.IsLatinLetter (cs/complex-condition) rewrites the block membership test as a single relational pattern instead of six chained `>= x && <= y` comparisons joined by `||`. The ranges, their order and the Unicode block comments are unchanged, so the result is identical for every char; the pattern form just states "letter is in one of these ranges" directly. Character literals replace the hex ints because a relational pattern needs a constant of the matched type, and the escaped form matches the char literals already used in this file. PhoneNumbers.MetadataBuilder.Main (cs/catch-of-all-exceptions) keeps its broad catch and gains a comment explaining why. It is the process-wide handler for a build-time tool over XML parsing, file I/O and named-mutex code, and it already writes the full exception to stderr and returns a non-zero exit code, so the build fails loudly. Narrowing it would only swap a readable diagnostic for an unhandled-exception crash dump. Deliberately not changed: - PhoneNumberUtil.cs (cs/missed-ternary-operator): the if/else there is guarded by a seventeen-line commented condition explaining the MX, CL and UZ formatting rules. Folding it into a ternary would strand the `?` and `:` arms far below the assignment target and make a block that exists mostly to carry that explanation harder to read. - PrefixFileReader.cs (cs/linq/missed-select): the loop body is not a projection. It filters malformed resource names, parses a country code, and mutates a dictionary; only its first line maps the iteration variable. Adding a `.Select` to the existing `.Where` would relocate that one line while adding a delegate and enumerator allocation on the geocoder/carrier/timezone construction path, which is startup cost this repo tracks.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
==========================================
+ Coverage 87.51% 87.54% +0.02%
==========================================
Files 43 43
Lines 3886 3885 -1
Branches 991 991
==========================================
Hits 3401 3401
Misses 280 280
+ Partials 205 204 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
TestIsLatinLetter asserted only Basic Latin, Latin-1 Supplement and a combining mark, so three arms of the range test - Latin Extended-A, Extended-B and Extended Additional - had no assertion reaching them. That showed up as a patch-coverage drop when the chained comparisons became a relational pattern, since the untested arms are now visible as their own branches. Add one letter from each missing block. This is coverage the test should always have had; the ranges themselves are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The four note-severity maintainability alerts. Two produced changes; two are deliberately declined, because a note-level style hint is not worth making the code worse.
Fixed
cs/complex-condition-PhoneNumberMatcher.cs:221.IsLatinLetter's six chainedletter >= x && letter <= yclauses joined by||became a single relational pattern —return letter is <= BASIC_LATIN or (>= ... and <= ...) or ...— one arm per Unicode block, using'\uXXXX'char literals.All operands are pure comparisons on a
char, so short-circuit order is irrelevant to behavior, andandbinds tighter thanorexactly as&&binds tighter than||- the original grouping is preserved. Ranges, their order, and the Unicode block comments are unchanged. Char literals replaced the hex ints because relational patterns require a constant of the matched type; that form already appears elsewhere in this file. Pattern combinators are already used elsewhere in the library (PrefixFileReader.MayFallBackToEnglish).cs/catch-of-all-exceptions-PhoneNumbers.MetadataBuilder/Program.cs:60. Kept, now documented. This is the process-wide handler inMainfor a build-time tool, and it was never a silent swallow: it writesex.Messageand the full exception to stderr and returns exit code 1, failing the invoking MSBuild target. The guarded work spans XML parsing, file I/O,GZipStreamand named-mutex operations, so a narrowed catch list would be a long guess that converts anything unanticipated into an unhandled-exception crash dump instead of a readable one-line diagnostic - with no change to whether the build fails. A comment now states that. The alert stays open; noquery-filtersexclusion was added for it.Declined
cs/missed-ternary-operator-PhoneNumberUtil.cs:1336. Theifis guarded by a ~17-line condition that is almost entirely comment text explaining the MX/CL/UZ and non-geo dialling rules. The branches are one line each, so a ternary would strand the?/:arms far below theformattedNumber =target at the bottom of a comment block that is the whole point of the construct. The if/else is clearer.cs/linq/missed-select-PrefixFileReader.cs:66. The loop body is not a projection: it filters malformed resource names (twocontinues), parses a country calling code, and mutates aSortedDictionary. Only its first line maps the iteration variable. This is a startup path -LoadFileNamesFromManifestResourcesis called from thePrefixFileReaderconstructor, which backsPhoneNumberOfflineGeocoder/PhoneNumberToCarrierMapper/PhoneNumberToTimeZonesMapperconstruction - and AGENTS.md documents cold-start and allocation cost as tracked here. Appending.Selectto the existing.Wherewould add a delegate plus another enumerator allocation on that path for zero readability gain.Verification
dotnet build csharp/PhoneNumbers.slnx -p:TargetFrameworks=net10.0- 0 warnings, 0 errors.PhoneNumbers.csprojalso built across all TFMs (netstandard2.0 + net8.0 + net10.0) to confirm the pattern syntax compiles on the netstandard2.0 leg.dotnet test csharp/PhoneNumbers.slnx -p:TargetFrameworks=net10.0- 491 passed, 0 failed, 0 skipped.TestPhoneNumberMatcherrun - 43 passed, includingTestIsLatinLetter, which covers Latin, accented, combining-mark, CJK, kana, digit and punctuation inputs.