AssemblyExtensionTypes - Reduce code complexity#6071
Conversation
|
Warning Rate limit exceeded@snakefoot has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 26 minutes and 54 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 (7)
WalkthroughPublic registration APIs were renamed from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/NLog/Config/AssemblyExtensionTypes.cssrc/NLog/Config/AssemblyExtensionTypes.ttsrc/NLog/Config/ConfigurationItemFactory.cssrc/NLog/Config/Factory.cssrc/NLog/Config/MethodFactory.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Config/Factory.cs (2)
src/NLog/Config/ConfigurationItemFactory.cs (1)
RegisterType(355-366)src/NLog/Config/IFactory.cs (1)
RegisterType(46-46)
🔇 Additional comments (8)
src/NLog/Config/MethodFactory.cs (1)
100-101: LGTM! Semantic inversion clarifies intent.The boolean logic has been correctly inverted:
checkTypeExists = truewhen items already exist, signaling that registrations should skip duplicates. This is clearer than the previousskipCheckExistssemantics.src/NLog/Config/ConfigurationItemFactory.cs (2)
144-250: LGTM! Consistent parameter renaming across all factory initializations.The lambda parameter has been systematically renamed from
skipCheckExiststocheckTypeExistsacross all factory initialization calls, maintaining consistency with the new semantics throughout the initialization flow.
474-559: LGTM! Registration methods consistently use the new parameter.All private registration methods now accept the
checkTypeExistsparameter and consistently pass it to the underlying factory registration calls, successfully consolidating the existence-check logic.src/NLog/Config/Factory.cs (3)
75-76: LGTM! Boolean inversion correctly implements new semantics.The inverted logic correctly sets
checkTypeExists = truewhen items already exist, enabling duplicate prevention in subsequent registrations.
85-96: LGTM! Registration with optional existence check is correctly implemented.The new
checkTypeExistsparameter (defaulting tofalse) allows callers to optionally prevent duplicate registrations. The early return logic correctly skips registration when both the flag is set and the type already exists, while maintaining backward compatibility.
134-162: LGTM! Consistent existence-check pattern for named type registration.The
RegisterNamedTypemethod mirrors the same optional existence-check pattern asRegisterType<TType>, ensuring consistency across registration paths.src/NLog/Config/AssemblyExtensionTypes.tt (1)
252-275: LGTM! Condition method guard logic correctly prevents duplicates.The registration pattern
if (!checkTypeExists || !conditionMethodFactory.CheckTypeAliasExists("..."))correctly ensures methods are registered when either existence checking is disabled or the method doesn't exist, effectively preventing duplicates while maintaining flexibility.src/NLog/Config/AssemblyExtensionTypes.cs (1)
251-274: LGTM! Generated guard logic correctly implements duplicate prevention.The condition method registration guards correctly implement the new existence-check pattern, preventing duplicates when
checkTypeExists = truewhile allowing unconditional registration whenfalse.
snakefoot
force-pushed
the
extension_registration
branch
2 times, most recently
from
3364b9e to
6ef5420
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/NLog/Config/AssemblyExtensionTypes.cs (1)
103-103: Inconsistent parameter usage in NETFRAMEWORK block.Line 103 doesn't pass the
checkTypeExistsparameter when registeringAppSettingLayoutRenderer. Since this is generated code, the fix should be applied to the template fileAssemblyExtensionTypes.ttat line 134.src/NLog/Config/AssemblyExtensionTypes.tt (1)
134-134: Inconsistent parameter usage in NETFRAMEWORK block.Line 134 doesn't pass the
checkTypeExistsparameter when registeringAppSettingLayoutRenderer, while line 65 does pass it forEventLogTarget. This inconsistency should be fixed to prevent potential duplicate registrations.🔎 Proposed fix
#if NETFRAMEWORK - layoutRendererFactory.RegisterType<NLog.LayoutRenderers.AppSettingLayoutRenderer>("appsetting"); + layoutRendererFactory.RegisterType<NLog.LayoutRenderers.AppSettingLayoutRenderer>("appsetting", checkTypeExists); #endif
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/NLog/Config/AssemblyExtensionTypes.cssrc/NLog/Config/AssemblyExtensionTypes.ttsrc/NLog/Config/ConfigurationItemFactory.cssrc/NLog/Config/Factory.cssrc/NLog/Config/MethodFactory.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/NLog/Config/Factory.cs
- src/NLog/Config/MethodFactory.cs
🧰 Additional context used
🧬 Code graph analysis (2)
src/NLog/Config/ConfigurationItemFactory.cs (3)
src/NLog/Config/Factory.cs (1)
Initialize(66-83)src/NLog/Config/MethodFactory.cs (1)
Initialize(91-108)src/NLog/Config/AssemblyExtensionTypes.cs (7)
AssemblyExtensionTypes(39-277)RegisterTargetTypes(42-77)RegisterLayoutTypes(79-95)RegisterLayoutRendererTypes(97-215)RegisterFilterTypes(217-226)RegisterConditionTypes(237-275)RegisterTimeSourceTypes(228-235)
src/NLog/Config/AssemblyExtensionTypes.cs (4)
src/NLog/Config/ConfigurationItemFactory.cs (6)
ConfigurationItemFactory(55-560)ConfigurationItemFactory(94-97)ConfigurationItemFactory(99-120)RegisterTypeProperties(368-380)RegisterTypeProperties(382-394)RegisterType(355-366)src/NLog/Config/Factory.cs (3)
RegisterType(85-96)RegisterType(98-107)RegisterType(114-127)src/NLog/Config/MethodFactory.cs (3)
RegisterOneParameter(312-320)RegisterTwoParameters(322-330)RegisterThreeParameters(332-340)src/NLog/Config/LoggingRule.cs (1)
ToString(309-354)
⏰ 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 (11)
src/NLog/Config/ConfigurationItemFactory.cs (3)
144-249: LGTM! Consistent refactoring of initialization lambdas.The initialization pattern has been cleanly updated across all factory types (TargetFactory, LayoutFactory, LayoutRendererFactory, AmbientRendererFactory, FilterFactory, TimeSourceFactory, ConditionMethodFactory) to use the new
checkTypeExistsparameter instead ofskipCheckExists. The changes are uniform and maintain the dependency chain initialization logic.
474-508: LGTM! RegisterAllTargets refactored consistently.The method signature and all internal registration calls have been updated to use the
checkTypeExistsparameter. All calls toRegisterNamedTypefor external package targets (DiagnosticSource, Database, WindowsEventLog, etc.) consistently pass the flag.
510-559: LGTM! All remaining RegisterAll methods refactored consistently.The RegisterAllLayouts, RegisterAllLayoutRenderers, RegisterAllFilters, RegisterAllConditionMethods, and RegisterAllTimeSources methods all follow the same refactored pattern with the
checkTypeExistsparameter properly threaded through to AssemblyExtensionTypes and all RegisterNamedType calls.src/NLog/Config/AssemblyExtensionTypes.cs (4)
42-77: LGTM! RegisterTargetTypes refactored cleanly.The method now accepts the
checkTypeExistsparameter and uses a localtargetFactoryvariable to centralize all registrations. All target types are registered consistently with the existence check flag, including the NETFRAMEWORK-specific EventLogTarget.
79-95: LGTM! RegisterLayoutTypes follows the refactored pattern.The method uses a local
layoutFactoryvariable and consistently passes thecheckTypeExistsparameter to all layout registrations.
217-235: LGTM! RegisterFilterTypes and RegisterTimeSourceTypes refactored correctly.Both methods introduce local factory variables and consistently use the
checkTypeExistsparameter for all type registrations.
237-275: LGTM! RegisterConditionTypes correctly implements conditional registration.The method uses
conditionMethodFactoryand implements proper guard logic withCheckTypeAliasExiststo prevent duplicate registrations of condition methods (length, equals, strequals, contains, starts-with, ends-with). The patternif (!checkTypeExists || !CheckTypeAliasExists(...))correctly ensures registrations only occur when appropriate.src/NLog/Config/AssemblyExtensionTypes.tt (4)
59-93: LGTM! RegisterTargetTypes template generates correct code.The template properly introduces the local
targetFactoryvariable and generates all registrations with thecheckTypeExistsparameter, including the NETFRAMEWORK-specific EventLogTarget.
95-126: LGTM! RegisterLayoutTypes template follows refactored pattern.The template correctly generates the method with local
layoutFactoryvariable and threads thecheckTypeExistsparameter through all layout registrations.
176-236: LGTM! RegisterFilterTypes and RegisterTimeSourceTypes templates are correct.Both template sections properly introduce local factory variables and generate registrations with the
checkTypeExistsparameter consistently threaded through.
238-276: LGTM! RegisterConditionTypes template implements correct conditional logic.The template generates proper guard conditions using
CheckTypeAliasExiststo prevent duplicate registrations of condition methods. The logicif (!checkTypeExists || !CheckTypeAliasExists(...))is correctly applied to all parameterized condition method registrations.
snakefoot
force-pushed
the
extension_registration
branch
6 times, most recently
from
4844858 to
55dc760
Compare
snakefoot
force-pushed
the
extension_registration
branch
from
55dc760 to
e3efd12
Compare
|
1 participant
No description provided.