LoggingConfiguration - Redirect AddRuleForOneLevel and AddRuleForAllLevels to AddRule#6010
LoggingConfiguration - Redirect AddRuleForOneLevel and AddRuleForAllLevels to AddRule#6010
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 14 minutes and 22 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)
WalkthroughLoggingConfiguration now ensures targets are registered before registering LoggingRule, centralizes AddRule overloads to funnel through common AddRule paths, and changes one target-not-found exception type. LoggingRule adds null-checks and conditional enabling for level ranges. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant LC as LoggingConfiguration
participant T as TargetRegistry
participant LR as LoggingRule
rect rgba(200,230,255,0.25)
note over App,LC: Overload delegation
App->>LC: AddRuleForOneLevel/AllLevels(...)
LC->>LC: Delegate to AddRule(minLevel,maxLevel, target|targetName, pattern, [final])
end
rect rgba(220,255,220,0.25)
note over LC,T: Target resolution & registration
LC->>T: Resolve target(s) (by name or object)
LC->>T: AddTargetThreadSafe(target) /* ensure registered */
LC->>LR: Create LoggingRule(...) /* using resolved targets */
LC->>LC: AddRule(LoggingRule) -> registers rule targets then adds rule
end
rect rgba(255,240,200,0.25)
note over App,LR: Level enabling validation
App->>LR: EnableLoggingForLevels(minLevel, maxLevel)
LR->>LR: Validate non-null and conditionally enable levels
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![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 (2)
src/NLog/Config/LoggingRule.cs (1)
242-244: Consistent null‑guards addedGood alignment with other range APIs and central validation via Guard.ThrowIfNull. Consider also clamping Off here as a defensive measure (e.g., treat maxLevel==Off as MaxLevel) to make the API more forgiving to callers, though the primary Off handling fix belongs at the call site in LoggingConfiguration (see comment there).
src/NLog/Config/LoggingConfiguration.cs (1)
346-350: Defensive null check when registering rule.TargetsIf a consumer accidentally inserts null into LoggingRule.Targets, AddTargetThreadSafe will dereference it. Guard inside the loop to prevent NREs.
Apply this diff:
- if (rule.Targets?.Count > 0) - { - foreach (var target in rule.Targets) - AddTargetThreadSafe(target); - } + if (rule.Targets?.Count > 0) + { + foreach (var target in rule.Targets) + { + if (target is null) + continue; // Ignore invalid entries defensively + AddTargetThreadSafe(target); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Config/LoggingConfiguration.cs(9 hunks)src/NLog/Config/LoggingRule.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/NLog/Config/LoggingConfiguration.cs (3)
src/NLog/NLogConfigurationException.cs (4)
NLogConfigurationException(51-53)NLogConfigurationException(59-62)NLogConfigurationException(98-101)NLogConfigurationException(115-118)src/NLog/Config/LoggingRule.cs (5)
LoggingRule(59-61)LoggingRule(66-69)LoggingRule(78-84)LoggingRule(92-98)LoggingRule(105-110)src/NLog/Internal/Guard.cs (1)
Guard(56-78)
src/NLog/Config/LoggingRule.cs (1)
src/NLog/Internal/Guard.cs (1)
Guard(56-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (3)
src/NLog/Config/LoggingConfiguration.cs (3)
306-306: Exception type switch to NLogConfigurationException is appropriateConfiguration error surfaced with a clearer exception type. Please confirm no external code relies on catching NLogRuntimeException here.
334-337: Registering target before adding rule improves consistencyTarget is now ensured to be known before the rule is stored, which helps later lookups and diagnostics. LGTM.
398-399: Centralizing AllLevels helpers through AddRule is cleanerReduces duplication and ensures consistent target registration and exception behavior. Nice refactor.
Also applies to: 408-409, 419-420
snakefoot
force-pushed
the
dev
branch
3 times, most recently
from
cf31dc7 to
e807900
Compare
|
1 participant
No description provided.