Repository navigation
Reduce AOT file-size by changing PropertyTypeConverter to avoid Dictionary for conversion mapping - #6174
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughRefactors string-to-type conversion to explicit per-type logic (PropertyTypeConverter + PropertyHelper), normalizes LoggingConfiguration variable expansion for null/whitespace inputs, and switches LogLevel backing collections to arrays. ChangesType Conversion Refactoring + Variable Expansion
🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/NLog/Config/PropertyTypeConverter.cs (1)
96-117: ⚡ Quick winCollapse the string-conversion allowlist into one source of truth.
HasConvertFromStringSupport(...)andTryConvertFromString(...)now have to stay manually aligned. If a future change adds support in only one place,IsComplexType(...)will disagree with the actual conversion path and you'll get subtle config parsing regressions.Also applies to: 121-187
🤖 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 `@src/NLog/Config/PropertyTypeConverter.cs` around lines 96 - 117, HasConvertFromStringSupport and TryConvertFromString (and IsComplexType) have duplicated allowlists that must stay manually aligned; consolidate them into a single source of truth (e.g. a private static readonly HashSet<Type> or a single helper method) that lists the supported convertible types and is consulted by HasConvertFromStringSupport, TryConvertFromString, and IsComplexType; ensure enum support (type.IsEnum) remains handled consistently and update all three callers (HasConvertFromStringSupport, TryConvertFromString, IsComplexType) to reference that single collection/helper so future changes are made in one place.
🤖 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.
Inline comments:
In `@src/NLog/Internal/PropertyHelper.cs`:
- Around line 283-287: The Layout<T> branch in PropertyHelper (the block
checking propertyType.IsGenericType && propertyType.GetGenericTypeDefinition()
== typeof(Layout<>)) incorrectly maps an explicit empty value (value == "") to
SimpleLayout.Default; change it to use the same empty-layout sentinel used by
the untyped Layout/SimpleLayout path when value is empty, so explicit ""
produces the empty-layout sentinel rather than SimpleLayout.Default before
calling Activator.CreateInstance to set newValue.
---
Nitpick comments:
In `@src/NLog/Config/PropertyTypeConverter.cs`:
- Around line 96-117: HasConvertFromStringSupport and TryConvertFromString (and
IsComplexType) have duplicated allowlists that must stay manually aligned;
consolidate them into a single source of truth (e.g. a private static readonly
HashSet<Type> or a single helper method) that lists the supported convertible
types and is consulted by HasConvertFromStringSupport, TryConvertFromString, and
IsComplexType; ensure enum support (type.IsEnum) remains handled consistently
and update all three callers (HasConvertFromStringSupport, TryConvertFromString,
IsComplexType) to reference that single collection/helper so future changes are
made in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 378d251b-a862-41d2-916f-d34452769196
📒 Files selected for processing (3)
src/NLog/Config/LoggingConfiguration.cssrc/NLog/Config/PropertyTypeConverter.cssrc/NLog/Internal/PropertyHelper.cs
snakefoot
force-pushed
the
objectconverter
branch
from
May 16, 2026 14:21
174d561 to
97865a6
Compare
…pping until needed
snakefoot
force-pushed
the
objectconverter
branch
from
May 16, 2026 14:43
97865a6 to
cdb55cb
Compare
|
This was referenced May 31, 2026
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.
Reducing upfront type-dependency resolving until actually needed (entering mapping-method)
Parsing Enum from empty string (or null) now fails upfront, instead of scanning for other conversions and casting.