Fix IsAcronymNumber and correctly group digit runs in acronym counting#4509
Fix IsAcronymNumber and correctly group digit runs in acronym counting#4509DavidGBrett wants to merge 3 commits into
Conversation
…scii value comparison Used IsAsciiDigit instead of IsDigit as that more closely replicates what originally existed and the other parts of this class don't seem ready to accept non ascii digits
📝 WalkthroughWalkthroughStringMatcher groups contiguous ASCII digit runs as single acronym units, counts distinct matched acronym groups for acronym scoring, replaces the manual digit check with char.IsAsciiDigit(...), and adds NUnit tests validating digit-run acronym behavior. ChangesAcronym grouping & digit detection
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Flow.Launcher.Infrastructure/StringMatcher.cs (2)
278-315: ⚡ Quick winEnsure consistent spacing and document precondition.
Minor style issues:
- Line 299:
groups+=1;→groups += 1;orgroups++;- Line 302: Missing space after comma in
IsAcronymNumber(stringToCompare,matchedIndex)- Line 307:
stringToCompare.Length-1→stringToCompare.Length - 1Additionally, the algorithm assumes
matchedIndicesis sorted ascending (currently satisfied because indices are added in loop order at line 142). Document this precondition in the XML comment or add a defensive sort.✨ Proposed fixes
/// <summary> /// Counts distinct acronym groups from matched character indices in <paramref name="stringToCompare"/>. +/// Assumes <paramref name="matchedIndices"/> is sorted in ascending order. /// Each matched non-digit character index forms its own group. /// Contiguous digit characters in the string form a single group regardless of how many indices match within the run. /// /// Example: /// For "Visual Studio 2019", matched indices [0, 14, 17] refer to characters 'V', '2', and '9'. /// These produce 2 groups: 'V' and the digit run "2019". /// </summary> private static int CountDistinctAcronymGroups(List<int> matchedIndices, string stringToCompare) { int groups = 0; var processedIndices = new HashSet<int>(); foreach (int matchedIndex in matchedIndices) { // try process index and skip if already processed in a previous group if (!processedIndices.Add(matchedIndex)) continue; // since we processed a new index we start a new group - groups+=1; + groups++; // if this isn't a digit then its a single index group so we stop here - if (!IsAcronymNumber(stringToCompare,matchedIndex)) + if (!IsAcronymNumber(stringToCompare, matchedIndex)) continue; // check if this is a digit run and process any indices in that run as they are part of this group int digitRunEnd = matchedIndex; - while (digitRunEnd < stringToCompare.Length-1 && IsAcronymNumber(stringToCompare, digitRunEnd + 1)) + while (digitRunEnd < stringToCompare.Length - 1 && IsAcronymNumber(stringToCompare, digitRunEnd + 1)) { digitRunEnd += 1; processedIndices.Add(digitRunEnd); } } return groups; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher.Infrastructure/StringMatcher.cs` around lines 278 - 315, The CountDistinctAcronymGroups method has minor style issues and an unstated precondition: fix spacing (use "groups += 1;" or "groups++;", add a space after the comma in IsAcronymNumber(stringToCompare, matchedIndex), and change "stringToCompare.Length-1" to "stringToCompare.Length - 1"), and either document that matchedIndices must be sorted ascending in the XML summary or defensively sort matchedIndices at the start of CountDistinctAcronymGroups (before using processedIndices) so the digit-run grouping logic using IsAcronymNumber and processedIndices behaves deterministically.
275-275: ⚡ Quick winRemove commented-out code.
Commented-out code is a maintenance burden. Since the fix is verified and tested, the old incorrect implementation should be deleted.
🧹 Proposed cleanup
private static bool IsAcronymNumber(string stringToCompare, int compareStringIndex) - // => stringToCompare[compareStringIndex] >= 0 && stringToCompare[compareStringIndex] <= 9; => char.IsAsciiDigit(stringToCompare[compareStringIndex]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher.Infrastructure/StringMatcher.cs` at line 275, Remove the leftover commented-out code line "// => stringToCompare[compareStringIndex] >= 0 && stringToCompare[compareStringIndex] <= 9;" from Flow.Launcher.Infrastructure.StringMatcher.cs; locate the comment inside the StringMatcher implementation (the code referencing stringToCompare and compareStringIndex) and delete that stale comment so only the current, verified logic remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Flow.Launcher.Infrastructure/StringMatcher.cs`:
- Around line 278-315: The CountDistinctAcronymGroups method has minor style
issues and an unstated precondition: fix spacing (use "groups += 1;" or
"groups++;", add a space after the comma in IsAcronymNumber(stringToCompare,
matchedIndex), and change "stringToCompare.Length-1" to "stringToCompare.Length
- 1"), and either document that matchedIndices must be sorted ascending in the
XML summary or defensively sort matchedIndices at the start of
CountDistinctAcronymGroups (before using processedIndices) so the digit-run
grouping logic using IsAcronymNumber and processedIndices behaves
deterministically.
- Line 275: Remove the leftover commented-out code line "// =>
stringToCompare[compareStringIndex] >= 0 && stringToCompare[compareStringIndex]
<= 9;" from Flow.Launcher.Infrastructure.StringMatcher.cs; locate the comment
inside the StringMatcher implementation (the code referencing stringToCompare
and compareStringIndex) and delete that stale comment so only the current,
verified logic remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e5763c7-36bd-4c53-93e0-119981fd94dc
📒 Files selected for processing (2)
Flow.Launcher.Infrastructure/StringMatcher.csFlow.Launcher.Test/FuzzyMatcherTest.cs
…ymQuery_ShouldReturnAcronymScore
d524d99 to
d7b0757
Compare
Fixes #4508
Replaces incorrect comparison of char to the intergers 0 and 9 (instead of the actual ASCII values of those characters) with char.IsAsciiDigit
Used IsAsciiDigit instead of IsDigit as that more closely replicates what originally existed and the other parts of this class don't seem ready to accept non ascii digits
Now the digits are actually being detected, we also need to fix the inconsistency where sometimes a digit run was treated as a single count in tallying but not in scoring - so score could exceed the 100 maximum.
Added a new CountDistinctAcronymGroups method to group digit runs into a single count, while leaving alphabetic acronyms as individual counts.
Summary by cubic
Improves acronym matching with numbers in
StringMatcherby using ASCII-only digit checks and grouping consecutive digits as one unit. This fixes scoring inflation and restores correct matches for names like "Visual Studio 2019".Summary of changes
IsAcronymNumbernow useschar.IsAsciiDigit. Acronym scoring usesCountDistinctAcronymGroupsso digit runs count once.CountDistinctAcronymGroupshelper; unit tests for digit-run acronyms inFlow.Launcher.Test/FuzzyMatcherTest.cs.char >= 0 && <= 9check; per-digit overcounting in acronym scoring.HashSetduring matching.Release Note
Abbreviation matching now treats number sequences like "2019" as a single part, improving search results and ranking.
Written for commit d7b0757. Summary will update on new commits.