ConsoleWordHighlightingRule - Support List of Words for simpler configuration#5954
ConsoleWordHighlightingRule - Support List of Words for simpler configuration#5954
Conversation
|
Warning Rate limit exceeded@snakefoot has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughMulti-word highlighting support was added to the Changes
Sequence Diagram(s)sequenceDiagram
participant Logger
participant ConsoleWordHighlightingRule
participant ConsoleOutput
Logger->>ConsoleWordHighlightingRule: Log message with text
alt Words property is set and not empty
ConsoleWordHighlightingRule->>ConsoleWordHighlightingRule: Find matches for each word in Words
ConsoleWordHighlightingRule->>ConsoleWordHighlightingRule: Merge overlapping matches
else
ConsoleWordHighlightingRule->>ConsoleWordHighlightingRule: Find matches for single Text
end
ConsoleWordHighlightingRule->>ConsoleOutput: Write highlighted message with ANSI codes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/NLog/Targets/ConsoleWordHighlightingRule.cs (4)
83-87: Add public-API metadata forWords.
Wordslacks the<docgen>attribute (and default value remarks) that other public properties expose. Adding it keeps XML docs & config-schema generation consistent.
122-133: Guard short-circuit logic withIsNullOrEmpty.Relying on
ReferenceEquals(_text, string.Empty)assumes the empty-string instance is always used.
Safer & clearer:- if (ReferenceEquals(_text, string.Empty)) + if (string.IsNullOrEmpty(_text))Avoids edge-cases if
Textis set to an empty span obtained via substring.
134-157: Possible quadratic growth when many words overlap.
YieldWordMatchesForHighlightingmerges matches word-by-word, each time copying/merging the accumulated list.
With n words and m matches per word this is O(n × m²) worst-case.If large word lists are expected, consider:
- Collect matches for all words → single
ListSortby start index- Single linear pass to merge overlaps
Reduces complexity to O(totalMatches log totalMatches).
226-242: Minor: avoid extra list copy when source already writable.
PrepareAllMatchesListcopies even whenallMatchesList.IsReadOnly == falsebut capacity insufficient.
List<T>automatically grows; returning the existing instance avoids allocation.tests/NLog.UnitTests/Targets/ColoredConsoleTargetTests.cs (1)
228-233: Mixed escape-sequence casing reduces consistency.Earlier tests use
\x1B; new expectations switch to lowercase\x1b. Functionally identical but stylistically uneven—prefer consistent casing across test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Targets/ConsoleWordHighlightingRule.cs(3 hunks)tests/NLog.UnitTests/Targets/ColoredConsoleTargetTests.cs(1 hunks)
🔇 Additional comments (2)
src/NLog/Targets/ConsoleWordHighlightingRule.cs (1)
210-216:NeedleMatchOverlapsoff-by-one definition check.Current test uses
>so ranges that merely touch (e.g.,[0,3]and[3,2]) are kept separate.
If highlighting should avoid duplicated resets between adjacent matches, change to>=.Request confirmation that the current behaviour is intentional.
tests/NLog.UnitTests/Targets/ColoredConsoleTargetTests.cs (1)
229-230: Extra reset\x1b[0mmay mask regressions.Expected string for CR/LF case contains two consecutive resets (
…big\x1b[0m\x1b[0m\r\n…).
If implementation later optimises to emit a single reset the test will fail spuriously. Consider asserting with a regex or trimming duplicate resets instead.
| private int FindNextWordForHighlighting(string needle, string haystack, int? prevIndex) | ||
| { | ||
| int index = prevIndex.HasValue ? prevIndex.Value + _text.Length : 0; | ||
| int index = prevIndex.HasValue ? prevIndex.Value + needle.Length : 0; | ||
| while (index >= 0) | ||
| { | ||
| index = IgnoreCase ? haystack.IndexOf(_text, index, System.StringComparison.CurrentCultureIgnoreCase) : haystack.IndexOf(_text, index); | ||
| if (index < 0 || (!WholeWords || StringHelpers.IsWholeWord(haystack, _text, index))) | ||
| index = IgnoreCase ? haystack.IndexOf(needle, index, System.StringComparison.CurrentCultureIgnoreCase) : haystack.IndexOf(needle, index); | ||
| if (index < 0 || (!WholeWords || StringHelpers.IsWholeWord(haystack, needle, index))) | ||
| return index; | ||
|
|
||
| index += _text.Length; | ||
| index += needle.Length; | ||
| } | ||
| return index; | ||
| } |
There was a problem hiding this comment.
Skip characters increment for partial-word rejection.
When WholeWords is true and a non-whole match is found, the search resumes after the entire needle (index += needle.Length).
This can skip valid overlapping whole-word matches such as searching “big” in “bigbig”. Incrementing by 1 avoids the blind spot:
- index += needle.Length;
+ index += 1;🤖 Prompt for AI Agents
In src/NLog/Targets/ConsoleWordHighlightingRule.cs around lines 272 to 284, when
WholeWords is true and a partial match is found, the code increments the index
by the length of the needle, which can skip overlapping whole-word matches. To
fix this, change the increment from needle.Length to 1 so the search advances by
one character instead of skipping ahead, ensuring all potential matches are
checked.
snakefoot
force-pushed
the
dev
branch
2 times, most recently
from
3643df0 to
7077467
Compare
|
snakefoot
added
needs documentation on wiki
documentation done
|
Updated wiki: https://github.com/NLog/NLog/wiki/ColoredConsole-target |
1 participant
With RegEx one could do the following:
Now one can do this (Remember to remove any leading/trailing white-spaces):