Replace ConventionMapper with typed config slices and SettingsBuilder#1156
Closed
christianhelle wants to merge 4 commits into
Closed
Replace ConventionMapper with typed config slices and SettingsBuilder#1156christianhelle wants to merge 4 commits into
christianhelle wants to merge 4 commits into
Conversation
Replace reflection-based ConventionMapper with compile-time-checked SettingsToBundleMapper that maps CLI Settings to typed config slices. Update SettingsMapper to compose bundle mapping + ToLegacySettings(). Remove ConventionMapper-specific tests and helper classes.
christianhelle
added
enhancement
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces a typed "sliced settings" architecture: eight sealed config slice records replace flat ChangesSliced Settings Architecture
Sequence Diagram(s)sequenceDiagram
participant CLI
participant SettingsToBundleMapper
participant SettingsBuilder
participant SettingsBundleExtensions
CLI->>SettingsToBundleMapper: Map settings to bundle
SettingsToBundleMapper->>SettingsBuilder: Register typed config slices
SettingsBuilder-->>SettingsToBundleMapper: Build and validate bundle
SettingsToBundleMapper-->>CLI: Return SettingsBundle
CLI->>SettingsBundleExtensions: Convert bundle to legacy settings
SettingsBundleExtensions-->>CLI: Return RefitGeneratorSettings
CLI->>CLI: Set remaining legacy fields
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Refitter.SourceGenerator/Refitter.SourceGenerator.csproj (1)
3-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnable
TreatWarningsAsErrorsin this production project.The project is under
src/**and currently does not enforce warning-free builds.As per coding guidelines, “
src/**/*.csproj: EnableTreatWarningsAsErrorson production projects.”🤖 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/Refitter.SourceGenerator/Refitter.SourceGenerator.csproj` around lines 3 - 15, The PropertyGroup in the Refitter.SourceGenerator.csproj file is missing the TreatWarningsAsErrors property which is required for production projects located under src/**. Add a new property element with TreatWarningsAsErrors set to true within the PropertyGroup section alongside the existing properties like EnforceExtendedAnalyzerRules and GenerateDocumentationFile to ensure the project enforces warning-free builds.Source: Coding guidelines
🧹 Nitpick comments (8)
src/Refitter.Core/Settings/Slices/OpenApiSourceConfigSlice.cs (1)
5-7: ⚡ Quick winDocument the public record with XML comments.
This new public API is missing XML documentation.
As per coding guidelines, "Include XML documentation for public APIs in C# code."
🤖 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/Refitter.Core/Settings/Slices/OpenApiSourceConfigSlice.cs` around lines 5 - 7, The public sealed record OpenApiSourceConfigSlice is missing XML documentation comments. Add XML comments above the record declaration to document its purpose, and include documentation for each property parameter (OpenApiPath and OpenApiPaths) using the standard C# XML documentation format with summary and param tags to explain what each property represents.Source: Coding guidelines
src/Refitter.Core/Settings/Slices/FilterConfigSlice.cs (1)
5-10: ⚡ Quick winAdd XML docs for this public configuration slice.
FilterConfigSliceis public and currently undocumented.As per coding guidelines, "Include XML documentation for public APIs in C# code."
🤖 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/Refitter.Core/Settings/Slices/FilterConfigSlice.cs` around lines 5 - 10, The FilterConfigSlice public record is missing XML documentation comments. Add XML documentation above the FilterConfigSlice record declaration to include a summary description of the record's purpose, and document each parameter with param tags explaining what IncludeTags, IncludePathMatches, IgnoredOperationHeaders, AdditionalNamespaces, and ExcludeNamespaces represent. Follow standard C# XML documentation conventions with triple-slash comments.Source: Coding guidelines
src/Refitter.Core/Settings/Slices/SchemaConfigSlice.cs (1)
5-8: ⚡ Quick winProvide XML documentation for the public record.
This public API is missing XML comments.
As per coding guidelines, "Include XML documentation for public APIs in C# code."
🤖 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/Refitter.Core/Settings/Slices/SchemaConfigSlice.cs` around lines 5 - 8, The SchemaConfigSlice public record is missing XML documentation comments required by the coding guidelines. Add XML documentation comments above the record declaration to describe its purpose, and add documentation comments for each property (TrimUnusedSchema, KeepSchemaPatterns, and IncludeInheritanceHierarchy) to explain what each configuration setting does. Use the standard triple-slash format (///) for XML documentation in C#.Source: Coding guidelines
src/Refitter.Core/Settings/Slices/OutputConfigSlice.cs (2)
5-11: ⚡ Quick winAdd XML documentation for this public slice record.
OutputConfigSliceis public but has no XML doc summary/param docs.As per coding guidelines, "Include XML documentation for public APIs in C# code."
🤖 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/Refitter.Core/Settings/Slices/OutputConfigSlice.cs` around lines 5 - 11, The public sealed record OutputConfigSlice lacks XML documentation comments. Add a summary XML documentation comment above the OutputConfigSlice record declaration that describes its purpose and responsibility. Additionally, add param documentation comments for each of the six properties: Namespace, ContractsNamespace, OutputFolder, ContractsOutputFolder, OutputFilename, and GenerateMultipleFiles, explaining what each property represents and its default value where applicable.Source: Coding guidelines
6-8: ⚡ Quick winUse legacy default constants instead of duplicated literals.
Line 6 and Line 8 hardcode defaults that also exist in legacy settings (
RefitGeneratorSettings.DefaultNamespace/RefitGeneratorSettings.DefaultOutputFolder). Reusing those constants prevents drift between the new slice path and legacy bridge behavior.🤖 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/Refitter.Core/Settings/Slices/OutputConfigSlice.cs` around lines 6 - 8, Replace the hardcoded default values in the OutputConfigSlice record with references to the existing legacy constants. For the Namespace property (which currently defaults to "GeneratedCode"), use RefitGeneratorSettings.DefaultNamespace instead. For the OutputFolder property (which currently defaults to "./Generated"), use RefitGeneratorSettings.DefaultOutputFolder instead. This ensures consistency between the new slice configuration path and the legacy bridge behavior by reusing the authoritative constant definitions.src/Refitter.Core/Settings/SettingsBuilder.cs (1)
7-52: ⚡ Quick winAdd XML docs for the new public builder API.
SettingsBuilderand its public members are exposed without XML documentation.As per coding guidelines, "Include XML documentation for public APIs in C# code."
🤖 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/Refitter.Core/Settings/SettingsBuilder.cs` around lines 7 - 52, The SettingsBuilder class and its public members (With<T>, Get<T>, TryGet<T>, and Build methods) lack XML documentation comments required by coding guidelines. Add XML documentation comments to the SettingsBuilder class with a summary of its purpose, and add summary, parameter, return value, and exception documentation to each public method (With<T>, Get<T>, TryGet<T>, and Build) to describe what they do, what parameters they accept, what they return, and what exceptions they may throw.Source: Coding guidelines
src/Refitter.Core/Settings/SettingsBundleExtensions.cs (1)
5-7: ⚡ Quick winDocument
SettingsBundleExtensionsandToLegacySettingswith XML comments.This introduces a public extension API without XML docs.
As per coding guidelines, "Include XML documentation for public APIs in C# code."
🤖 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/Refitter.Core/Settings/SettingsBundleExtensions.cs` around lines 5 - 7, Add XML documentation comments to the public class SettingsBundleExtensions and the public method ToLegacySettings. For the class, provide a summary explaining that it contains extension methods for SettingsBundle conversion. For the ToLegacySettings method, include a summary describing what the method does, a param tag documenting the bundle parameter, and a returns tag describing what the method returns (the converted RefitGeneratorSettings). This ensures the public API has proper documentation following C# coding guidelines.Source: Coding guidelines
src/Refitter/SettingsMapper.cs (1)
12-14: ⚡ Quick winRemove redundant
OpenApiPathremap after bundle conversion.
ToLegacySettings()already maps OpenAPI source fields, so Line 13 duplicates that work and keeps an unnecessary null-forgiving path in this method.Suggested cleanup
- // Properties not covered by config slices - result.OpenApiPath = settings.OpenApiPath!; + // Properties not covered by config slices result.ApizrSettings = settings.UseApizr ? new ApizrSettings() : null;🤖 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/Refitter/SettingsMapper.cs` around lines 12 - 14, Remove the redundant line assigning `result.OpenApiPath = settings.OpenApiPath!;` from the SettingsMapper method since the `ToLegacySettings()` method already handles the mapping of OpenAPI source fields. This will eliminate the duplicate work and remove the unnecessary null-forgiving operator from this location.
🤖 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/Refitter.Core/Settings/SettingsBuilder.cs`:
- Around line 11-15: The With<T>() method in the SettingsBuilder class accepts
null values for the slice parameter and adds them to the slices dictionary
without validation, which can cause silent invalid registrations and
harder-to-diagnose failures later. Add a null check at the beginning of the
With<T>() method to validate that the slice parameter is not null, and throw an
appropriate exception such as ArgumentNullException if a null value is provided,
preventing invalid registrations from being stored.
In `@src/Refitter.Core/Settings/SettingsBundle.cs`:
- Around line 8-30: The public methods Get<T>() and TryGet<T>() in the
SettingsBundle class are missing XML documentation comments. Add XML
documentation comments above both methods that include a summary description
explaining what each method does, documentation for the generic type parameter T
(where applicable), return value descriptions, and for Get<T>() specifically,
document the KeyNotFoundException that can be thrown. Use the standard C# XML
documentation format with summary, param, returns, and exception tags as
appropriate for each method.
- Line 6: The SettingsBundle record is storing a direct reference to the
caller-provided IReadOnlyDictionary<Type, object> Slices parameter, which allows
the underlying mutable source to be modified after construction. To fix this,
create a defensive snapshot of the incoming dictionary when storing it in the
SettingsBundle record. This can be done by converting the incoming Slices
parameter to an immutable dictionary (such as using .ToImmutableDictionary() or
creating a new Dictionary copy) within the record definition or constructor,
ensuring that any post-construction mutations to the caller's original
dictionary do not affect the SettingsBundle instance.
In `@src/Refitter.Core/Settings/SettingsResult.cs`:
- Around line 10-16: The IsValid property in SettingsResult only validates that
Errors is null or empty, but does not check whether Bundle is null. Update the
IsValid property definition to include an additional condition that ensures
Bundle is not null, so that IsValid returns true only when both the Errors
collection is empty AND Bundle has been properly initialized. This prevents null
Bundle values from being pushed downstream when ThrowIfInvalid() is called.
- Around line 6-17: The SettingsResult record and all of its public members lack
XML documentation. Add XML documentation comments (using ///) above the
SettingsResult record declaration to describe its purpose, then document each
public member including the Errors and Bundle constructor parameters, the
IsValid property, and the ThrowIfInvalid() method. Each documentation comment
should clearly explain what the member does and its role in the API surface.
In `@src/Refitter.Core/Settings/SettingsValidationException.cs`:
- Around line 6-14: Add XML documentation comments to the
SettingsValidationException class and its public members. Include a summary
comment for the SettingsValidationException class itself describing what this
exception represents, add a summary comment for the ValidationErrors property
explaining what it contains, and add a summary comment plus a parameter
description for the constructor documenting what the errors parameter represents
and how it is used in the exception message.
In `@src/Refitter.Core/Settings/Slices/FeatureConfigSlice.cs`:
- Around line 5-9: The public record FeatureConfigSlice is missing XML
documentation comments required by coding guidelines. Add a summary XML comment
above the FeatureConfigSlice record declaration describing its purpose and
provide param documentation for each property (UsePolymorphicSerialization,
AuthenticationHeaderStyle, SecurityScheme, and GenerateJsonSerializerContext)
explaining what each configuration option controls.
In `@src/Refitter.Core/Settings/Slices/GenerationConfig.cs`:
- Line 23: The ResponseTypeOverride property in the GenerationConfig record
exposes a mutable Dictionary<string, string> which breaks immutability
guarantees of the record, as external code can modify the dictionary after
construction. Replace the mutable Dictionary type with an immutable collection
type such as ImmutableDictionary<string, string> from the
System.Collections.Immutable namespace, or use IReadOnlyDictionary<string,
string> to ensure the configuration state cannot be unexpectedly altered through
external mutation of the dictionary.
- Around line 6-24: The public sealed record GenerationConfig is missing XML
documentation which impacts IntelliSense and API usability. Add XML
documentation comments (/// summary sections) above the GenerationConfig record
declaration and document each of the significant parameters to describe their
purpose and behavior. Include high-level documentation for the record itself and
meaningful descriptions for each property parameter to help developers
understand the configuration options available.
In `@src/Refitter.Core/Settings/Slices/ParameterConfigSlice.cs`:
- Around line 5-10: The ParameterConfigSlice record and its public properties
are missing XML documentation comments required by C# coding guidelines. Add XML
documentation comments using the /// syntax above the ParameterConfigSlice
record declaration to describe its purpose, and add documentation comments for
each of the public properties: UseCancellationTokens, UseIsoDateFormat,
OptionalParameters, UseDynamicQuerystringParameters, and CollectionFormat. Each
property documentation should explain what configuration it controls and its
default behavior.
In `@src/Refitter.Core/Settings/Slices/TypeConfigSlice.cs`:
- Around line 5-9: The public sealed record TypeConfigSlice is missing XML
documentation comments required by coding guidelines. Add XML documentation
comments above the TypeConfigSlice record declaration that includes a summary
element describing the purpose of the record, and individual param elements
documenting each parameter (TypeAccessibility, PropertyNamingPolicy,
ImmutableRecords, and ContractTypeSuffix) to explain their purposes and default
values.
In `@src/Refitter/SettingsToBundleMapper.cs`:
- Around line 36-45: The OutputConfigSlice configuration in the builder.With
method hardcodes OutputFolder to "./Generated" and uses settings.OutputPath only
as a fallback for ContractsOutputFolder, effectively ignoring the user's
OutputPath setting for the primary client output. Fix this by changing
OutputFolder to use settings.OutputPath (with "./Generated" as the default
fallback if OutputPath is null or empty), and update ContractsOutputFolder to
use settings.ContractsOutputPath with an appropriate fallback that does not
depend on the primary OutputPath setting. This ensures the user's selected
client output destination is respected.
---
Outside diff comments:
In `@src/Refitter.SourceGenerator/Refitter.SourceGenerator.csproj`:
- Around line 3-15: The PropertyGroup in the Refitter.SourceGenerator.csproj
file is missing the TreatWarningsAsErrors property which is required for
production projects located under src/**. Add a new property element with
TreatWarningsAsErrors set to true within the PropertyGroup section alongside the
existing properties like EnforceExtendedAnalyzerRules and
GenerateDocumentationFile to ensure the project enforces warning-free builds.
---
Nitpick comments:
In `@src/Refitter.Core/Settings/SettingsBuilder.cs`:
- Around line 7-52: The SettingsBuilder class and its public members (With<T>,
Get<T>, TryGet<T>, and Build methods) lack XML documentation comments required
by coding guidelines. Add XML documentation comments to the SettingsBuilder
class with a summary of its purpose, and add summary, parameter, return value,
and exception documentation to each public method (With<T>, Get<T>, TryGet<T>,
and Build) to describe what they do, what parameters they accept, what they
return, and what exceptions they may throw.
In `@src/Refitter.Core/Settings/SettingsBundleExtensions.cs`:
- Around line 5-7: Add XML documentation comments to the public class
SettingsBundleExtensions and the public method ToLegacySettings. For the class,
provide a summary explaining that it contains extension methods for
SettingsBundle conversion. For the ToLegacySettings method, include a summary
describing what the method does, a param tag documenting the bundle parameter,
and a returns tag describing what the method returns (the converted
RefitGeneratorSettings). This ensures the public API has proper documentation
following C# coding guidelines.
In `@src/Refitter.Core/Settings/Slices/FilterConfigSlice.cs`:
- Around line 5-10: The FilterConfigSlice public record is missing XML
documentation comments. Add XML documentation above the FilterConfigSlice record
declaration to include a summary description of the record's purpose, and
document each parameter with param tags explaining what IncludeTags,
IncludePathMatches, IgnoredOperationHeaders, AdditionalNamespaces, and
ExcludeNamespaces represent. Follow standard C# XML documentation conventions
with triple-slash comments.
In `@src/Refitter.Core/Settings/Slices/OpenApiSourceConfigSlice.cs`:
- Around line 5-7: The public sealed record OpenApiSourceConfigSlice is missing
XML documentation comments. Add XML comments above the record declaration to
document its purpose, and include documentation for each property parameter
(OpenApiPath and OpenApiPaths) using the standard C# XML documentation format
with summary and param tags to explain what each property represents.
In `@src/Refitter.Core/Settings/Slices/OutputConfigSlice.cs`:
- Around line 5-11: The public sealed record OutputConfigSlice lacks XML
documentation comments. Add a summary XML documentation comment above the
OutputConfigSlice record declaration that describes its purpose and
responsibility. Additionally, add param documentation comments for each of the
six properties: Namespace, ContractsNamespace, OutputFolder,
ContractsOutputFolder, OutputFilename, and GenerateMultipleFiles, explaining
what each property represents and its default value where applicable.
- Around line 6-8: Replace the hardcoded default values in the OutputConfigSlice
record with references to the existing legacy constants. For the Namespace
property (which currently defaults to "GeneratedCode"), use
RefitGeneratorSettings.DefaultNamespace instead. For the OutputFolder property
(which currently defaults to "./Generated"), use
RefitGeneratorSettings.DefaultOutputFolder instead. This ensures consistency
between the new slice configuration path and the legacy bridge behavior by
reusing the authoritative constant definitions.
In `@src/Refitter.Core/Settings/Slices/SchemaConfigSlice.cs`:
- Around line 5-8: The SchemaConfigSlice public record is missing XML
documentation comments required by the coding guidelines. Add XML documentation
comments above the record declaration to describe its purpose, and add
documentation comments for each property (TrimUnusedSchema, KeepSchemaPatterns,
and IncludeInheritanceHierarchy) to explain what each configuration setting
does. Use the standard triple-slash format (///) for XML documentation in C#.
In `@src/Refitter/SettingsMapper.cs`:
- Around line 12-14: Remove the redundant line assigning `result.OpenApiPath =
settings.OpenApiPath!;` from the SettingsMapper method since the
`ToLegacySettings()` method already handles the mapping of OpenAPI source
fields. This will eliminate the duplicate work and remove the unnecessary
null-forgiving operator from this location.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e82efea-ee0c-4d6c-8ace-57b31e72617b
📒 Files selected for processing (18)
src/Refitter.Core/Settings/SettingsBuilder.cssrc/Refitter.Core/Settings/SettingsBundle.cssrc/Refitter.Core/Settings/SettingsBundleExtensions.cssrc/Refitter.Core/Settings/SettingsResult.cssrc/Refitter.Core/Settings/SettingsValidationException.cssrc/Refitter.Core/Settings/Slices/FeatureConfigSlice.cssrc/Refitter.Core/Settings/Slices/FilterConfigSlice.cssrc/Refitter.Core/Settings/Slices/GenerationConfig.cssrc/Refitter.Core/Settings/Slices/OpenApiSourceConfigSlice.cssrc/Refitter.Core/Settings/Slices/OutputConfigSlice.cssrc/Refitter.Core/Settings/Slices/ParameterConfigSlice.cssrc/Refitter.Core/Settings/Slices/SchemaConfigSlice.cssrc/Refitter.Core/Settings/Slices/TypeConfigSlice.cssrc/Refitter.SourceGenerator/Refitter.SourceGenerator.csprojsrc/Refitter.Tests/SettingsMapperTests.cssrc/Refitter/ConventionMapper.cssrc/Refitter/SettingsMapper.cssrc/Refitter/SettingsToBundleMapper.cs
💤 Files with no reviewable changes (2)
- src/Refitter/ConventionMapper.cs
- src/Refitter.Tests/SettingsMapperTests.cs
Comment on lines
+11
to
+15
| public SettingsBuilder With<T>(T slice) | ||
| where T : class | ||
| { | ||
| slices[typeof(T)] = slice; | ||
| return this; |
Contributor
There was a problem hiding this comment.
Reject null slices in With<T>() to avoid silent invalid registrations.
Line 14 allows null to be registered as a slice, which can propagate incomplete state into Build() and make failures harder to diagnose later.
Suggested fix
public SettingsBuilder With<T>(T slice)
where T : class
{
+ if (slice is null)
+ throw new ArgumentNullException(nameof(slice));
+
slices[typeof(T)] = slice;
return this;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public SettingsBuilder With<T>(T slice) | |
| where T : class | |
| { | |
| slices[typeof(T)] = slice; | |
| return this; | |
| public SettingsBuilder With<T>(T slice) | |
| where T : class | |
| { | |
| if (slice is null) | |
| throw new ArgumentNullException(nameof(slice)); | |
| slices[typeof(T)] = slice; | |
| return this; | |
| } |
🤖 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/Refitter.Core/Settings/SettingsBuilder.cs` around lines 11 - 15, The
With<T>() method in the SettingsBuilder class accepts null values for the slice
parameter and adds them to the slices dictionary without validation, which can
cause silent invalid registrations and harder-to-diagnose failures later. Add a
null check at the beginning of the With<T>() method to validate that the slice
parameter is not null, and throw an appropriate exception such as
ArgumentNullException if a null value is provided, preventing invalid
registrations from being stored.
|
|
||
| namespace Refitter.Core; | ||
|
|
||
| public sealed record SettingsBundle(IReadOnlyDictionary<Type, object> Slices) |
Contributor
There was a problem hiding this comment.
SettingsBundle should defensively snapshot incoming slices.
Storing caller-provided dictionary by reference allows post-construction mutation and undermines immutability guarantees.
🤖 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/Refitter.Core/Settings/SettingsBundle.cs` at line 6, The SettingsBundle
record is storing a direct reference to the caller-provided
IReadOnlyDictionary<Type, object> Slices parameter, which allows the underlying
mutable source to be modified after construction. To fix this, create a
defensive snapshot of the incoming dictionary when storing it in the
SettingsBundle record. This can be done by converting the incoming Slices
parameter to an immutable dictionary (such as using .ToImmutableDictionary() or
creating a new Dictionary copy) within the record definition or constructor,
ensuring that any post-construction mutations to the caller's original
dictionary do not affect the SettingsBundle instance.
Comment on lines
+8
to
+30
| public T Get<T>() | ||
| where T : class | ||
| { | ||
| if (Slices.TryGetValue(typeof(T), out var slice)) | ||
| return (T)slice; | ||
|
|
||
| throw new KeyNotFoundException( | ||
| $"Config slice of type {typeof(T).Name} has not been registered. " + | ||
| "Call SettingsBuilder.With<T>() to register the slice before building."); | ||
| } | ||
|
|
||
| public bool TryGet<T>(out T slice) | ||
| where T : class | ||
| { | ||
| if (Slices.TryGetValue(typeof(T), out var raw) && raw is T typed) | ||
| { | ||
| slice = typed; | ||
| return true; | ||
| } | ||
|
|
||
| slice = default!; | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Add XML docs for Get<T>() / TryGet<T>() public APIs.
Both public methods are currently undocumented.
As per coding guidelines, “Include XML documentation for public APIs in C# code.”
🤖 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/Refitter.Core/Settings/SettingsBundle.cs` around lines 8 - 30, The public
methods Get<T>() and TryGet<T>() in the SettingsBundle class are missing XML
documentation comments. Add XML documentation comments above both methods that
include a summary description explaining what each method does, documentation
for the generic type parameter T (where applicable), return value descriptions,
and for Get<T>() specifically, document the KeyNotFoundException that can be
thrown. Use the standard C# XML documentation format with summary, param,
returns, and exception tags as appropriate for each method.
Source: Coding guidelines
Comment on lines
+6
to
+17
| public sealed record SettingsResult( | ||
| IReadOnlyList<string>? Errors, | ||
| SettingsBundle? Bundle = null) | ||
| { | ||
| public bool IsValid => Errors is null || Errors.Count == 0; | ||
|
|
||
| public void ThrowIfInvalid() | ||
| { | ||
| if (!IsValid) | ||
| throw new SettingsValidationException(Errors!); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add XML documentation for this public result type.
SettingsResult and its public API surface are undocumented.
As per coding guidelines, “Include XML documentation for public APIs in C# code.”
🤖 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/Refitter.Core/Settings/SettingsResult.cs` around lines 6 - 17, The
SettingsResult record and all of its public members lack XML documentation. Add
XML documentation comments (using ///) above the SettingsResult record
declaration to describe its purpose, then document each public member including
the Errors and Bundle constructor parameters, the IsValid property, and the
ThrowIfInvalid() method. Each documentation comment should clearly explain what
the member does and its role in the API surface.
Source: Coding guidelines
Comment on lines
+10
to
+16
| public bool IsValid => Errors is null || Errors.Count == 0; | ||
|
|
||
| public void ThrowIfInvalid() | ||
| { | ||
| if (!IsValid) | ||
| throw new SettingsValidationException(Errors!); | ||
| } |
Contributor
There was a problem hiding this comment.
IsValid can return true while Bundle is null.
With Errors == null and Bundle == null, IsValid is true and ThrowIfInvalid() won’t throw, which can push a null bundle downstream.
🤖 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/Refitter.Core/Settings/SettingsResult.cs` around lines 10 - 16, The
IsValid property in SettingsResult only validates that Errors is null or empty,
but does not check whether Bundle is null. Update the IsValid property
definition to include an additional condition that ensures Bundle is not null,
so that IsValid returns true only when both the Errors collection is empty AND
Bundle has been properly initialized. This prevents null Bundle values from
being pushed downstream when ThrowIfInvalid() is called.
Comment on lines
+6
to
+24
| public sealed record GenerationConfig( | ||
| [property: JsonPropertyName("generateContracts")] bool GenerateContracts = true, | ||
| [property: JsonPropertyName("generateClients")] bool GenerateClients = true, | ||
| [property: JsonPropertyName("generateDisposableClients")] bool GenerateDisposableClients = false, | ||
| [property: JsonPropertyName("generateDeprecatedOperations")] bool GenerateDeprecatedOperations = true, | ||
| [property: JsonPropertyName("generateDefaultAdditionalProperties")] bool GenerateDefaultAdditionalProperties = true, | ||
| [property: JsonPropertyName("multipleInterfaces")] MultipleInterfaces MultipleInterfaces = default, | ||
| [property: JsonPropertyName("operationNameTemplate")] string? OperationNameTemplate = null, | ||
| [property: JsonPropertyName("operationNameGenerator")] OperationNameGeneratorTypes OperationNameGenerator = default, | ||
| [property: JsonPropertyName("generateOperationHeaders")] bool GenerateOperationHeaders = true, | ||
| [property: JsonPropertyName("generateXmlDocCodeComments")] bool GenerateXmlDocCodeComments = true, | ||
| [property: JsonPropertyName("generateStatusCodeComments")] bool GenerateStatusCodeComments = true, | ||
| [property: JsonPropertyName("addAutoGeneratedHeader")] bool AddAutoGeneratedHeader = true, | ||
| [property: JsonPropertyName("addAcceptHeaders")] bool AddAcceptHeaders = true, | ||
| [property: JsonPropertyName("addContentTypeHeaders")] bool AddContentTypeHeaders = true, | ||
| [property: JsonPropertyName("returnIApiResponse")] bool ReturnIApiResponse = false, | ||
| [property: JsonPropertyName("returnIObservable")] bool ReturnIObservable = false, | ||
| [property: JsonPropertyName("responseTypeOverride")] Dictionary<string, string>? ResponseTypeOverride = null, | ||
| [property: JsonPropertyName("customTemplateDirectory")] string? CustomTemplateDirectory = null); |
Contributor
There was a problem hiding this comment.
Add XML documentation to this public record API.
GenerationConfig is public but has no XML docs, which makes IntelliSense/API usage harder to validate and maintain.
As per coding guidelines, “Include XML documentation for public APIs in C# code.”
🤖 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/Refitter.Core/Settings/Slices/GenerationConfig.cs` around lines 6 - 24,
The public sealed record GenerationConfig is missing XML documentation which
impacts IntelliSense and API usability. Add XML documentation comments (///
summary sections) above the GenerationConfig record declaration and document
each of the significant parameters to describe their purpose and behavior.
Include high-level documentation for the record itself and meaningful
descriptions for each property parameter to help developers understand the
configuration options available.
Source: Coding guidelines
| [property: JsonPropertyName("addContentTypeHeaders")] bool AddContentTypeHeaders = true, | ||
| [property: JsonPropertyName("returnIApiResponse")] bool ReturnIApiResponse = false, | ||
| [property: JsonPropertyName("returnIObservable")] bool ReturnIObservable = false, | ||
| [property: JsonPropertyName("responseTypeOverride")] Dictionary<string, string>? ResponseTypeOverride = null, |
Contributor
There was a problem hiding this comment.
ResponseTypeOverride breaks effective immutability of the slice.
Line 23 exposes a mutable Dictionary<string, string>? from an “immutable” record. External mutation after construction can change config state unexpectedly.
🤖 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/Refitter.Core/Settings/Slices/GenerationConfig.cs` at line 23, The
ResponseTypeOverride property in the GenerationConfig record exposes a mutable
Dictionary<string, string> which breaks immutability guarantees of the record,
as external code can modify the dictionary after construction. Replace the
mutable Dictionary type with an immutable collection type such as
ImmutableDictionary<string, string> from the System.Collections.Immutable
namespace, or use IReadOnlyDictionary<string, string> to ensure the
configuration state cannot be unexpectedly altered through external mutation of
the dictionary.
Comment on lines
+5
to
+10
| public sealed record ParameterConfigSlice( | ||
| [property: JsonPropertyName("useCancellationTokens")] bool UseCancellationTokens = false, | ||
| [property: JsonPropertyName("useIsoDateFormat")] bool UseIsoDateFormat = false, | ||
| [property: JsonPropertyName("optionalParameters")] bool OptionalParameters = false, | ||
| [property: JsonPropertyName("useDynamicQuerystringParameters")] bool UseDynamicQuerystringParameters = false, | ||
| [property: JsonPropertyName("collectionFormat")] CollectionFormat CollectionFormat = CollectionFormat.Multi); |
Contributor
There was a problem hiding this comment.
Add XML docs for ParameterConfigSlice public API.
Public config surface is missing XML documentation.
As per coding guidelines, “Include XML documentation for public APIs in C# code.”
🤖 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/Refitter.Core/Settings/Slices/ParameterConfigSlice.cs` around lines 5 -
10, The ParameterConfigSlice record and its public properties are missing XML
documentation comments required by C# coding guidelines. Add XML documentation
comments using the /// syntax above the ParameterConfigSlice record declaration
to describe its purpose, and add documentation comments for each of the public
properties: UseCancellationTokens, UseIsoDateFormat, OptionalParameters,
UseDynamicQuerystringParameters, and CollectionFormat. Each property
documentation should explain what configuration it controls and its default
behavior.
Source: Coding guidelines
Comment on lines
+5
to
+9
| public sealed record TypeConfigSlice( | ||
| [property: JsonPropertyName("typeAccessibility")] TypeAccessibility TypeAccessibility = TypeAccessibility.Public, | ||
| [property: JsonPropertyName("propertyNamingPolicy")] PropertyNamingPolicy PropertyNamingPolicy = PropertyNamingPolicy.PascalCase, | ||
| [property: JsonPropertyName("immutableRecords")] bool ImmutableRecords = false, | ||
| [property: JsonPropertyName("contractTypeSuffix")] string? ContractTypeSuffix = null); |
Contributor
There was a problem hiding this comment.
Document the public TypeConfigSlice API with XML comments.
This public record currently has no XML documentation.
As per coding guidelines, “Include XML documentation for public APIs in C# code.”
🤖 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/Refitter.Core/Settings/Slices/TypeConfigSlice.cs` around lines 5 - 9, The
public sealed record TypeConfigSlice is missing XML documentation comments
required by coding guidelines. Add XML documentation comments above the
TypeConfigSlice record declaration that includes a summary element describing
the purpose of the record, and individual param elements documenting each
parameter (TypeAccessibility, PropertyNamingPolicy, ImmutableRecords, and
ContractTypeSuffix) to explain their purposes and default values.
Source: Coding guidelines
Comment on lines
+36
to
+45
| builder.With(new OutputConfigSlice | ||
| { | ||
| Namespace = settings.Namespace ?? "GeneratedCode", | ||
| ContractsNamespace = settings.ContractsNamespace, | ||
| OutputFolder = "./Generated", | ||
| ContractsOutputFolder = settings.ContractsOutputPath ?? settings.OutputPath, | ||
| OutputFilename = null, | ||
| GenerateMultipleFiles = settings.GenerateMultipleFiles | ||
| || !string.IsNullOrWhiteSpace(settings.ContractsOutputPath), | ||
| }); |
Contributor
There was a problem hiding this comment.
settings.OutputPath is effectively dropped from primary output mapping.
Line 40 and Line 42 force defaults for OutputFolder/OutputFilename, while Line 41 redirects settings.OutputPath into ContractsOutputFolder. That can ignore the user-selected client output destination.
Suggested fix direction
+using System.IO;
using Refitter.Core;
using Refitter.Core.Settings;
@@
builder.With(new OutputConfigSlice
{
Namespace = settings.Namespace ?? "GeneratedCode",
ContractsNamespace = settings.ContractsNamespace,
- OutputFolder = "./Generated",
- ContractsOutputFolder = settings.ContractsOutputPath ?? settings.OutputPath,
- OutputFilename = null,
+ OutputFolder = !string.IsNullOrWhiteSpace(settings.OutputPath)
+ ? (Path.GetDirectoryName(settings.OutputPath) ?? "./Generated")
+ : "./Generated",
+ ContractsOutputFolder = settings.ContractsOutputPath,
+ OutputFilename = !string.IsNullOrWhiteSpace(settings.OutputPath)
+ ? Path.GetFileName(settings.OutputPath)
+ : null,
GenerateMultipleFiles = settings.GenerateMultipleFiles
|| !string.IsNullOrWhiteSpace(settings.ContractsOutputPath),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| builder.With(new OutputConfigSlice | |
| { | |
| Namespace = settings.Namespace ?? "GeneratedCode", | |
| ContractsNamespace = settings.ContractsNamespace, | |
| OutputFolder = "./Generated", | |
| ContractsOutputFolder = settings.ContractsOutputPath ?? settings.OutputPath, | |
| OutputFilename = null, | |
| GenerateMultipleFiles = settings.GenerateMultipleFiles | |
| || !string.IsNullOrWhiteSpace(settings.ContractsOutputPath), | |
| }); | |
| using System.IO; | |
| using Refitter.Core; | |
| using Refitter.Core.Settings; | |
| builder.With(new OutputConfigSlice | |
| { | |
| Namespace = settings.Namespace ?? "GeneratedCode", | |
| ContractsNamespace = settings.ContractsNamespace, | |
| OutputFolder = !string.IsNullOrWhiteSpace(settings.OutputPath) | |
| ? (Path.GetDirectoryName(settings.OutputPath) ?? "./Generated") | |
| : "./Generated", | |
| ContractsOutputFolder = settings.ContractsOutputPath, | |
| OutputFilename = !string.IsNullOrWhiteSpace(settings.OutputPath) | |
| ? Path.GetFileName(settings.OutputPath) | |
| : null, | |
| GenerateMultipleFiles = settings.GenerateMultipleFiles | |
| || !string.IsNullOrWhiteSpace(settings.ContractsOutputPath), | |
| }); |
🤖 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/Refitter/SettingsToBundleMapper.cs` around lines 36 - 45, The
OutputConfigSlice configuration in the builder.With method hardcodes
OutputFolder to "./Generated" and uses settings.OutputPath only as a fallback
for ContractsOutputFolder, effectively ignoring the user's OutputPath setting
for the primary client output. Fix this by changing OutputFolder to use
settings.OutputPath (with "./Generated" as the default fallback if OutputPath is
null or empty), and update ContractsOutputFolder to use
settings.ContractsOutputPath with an appropriate fallback that does not depend
on the primary OutputPath setting. This ensures the user's selected client
output destination is respected.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1156 +/- ##
==========================================
- Coverage 94.74% 94.61% -0.13%
==========================================
Files 69 82 +13
Lines 3156 3379 +223
==========================================
+ Hits 2990 3197 +207
+ Misses 62 60 -2
- Partials 104 122 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
christianhelle
changed the title
…extension methods
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Refitter.Tests/SettingsBuilderTests.cs`:
- Line 6: The namespace in SettingsBuilderTests.cs is currently Refitter.Tests
but project conventions require new-feature tests to use the
Refitter.Tests.Scenarios namespace. Update the namespace declaration at line 6
from "namespace Refitter.Tests;" to "namespace Refitter.Tests.Scenarios;" and
ensure the test class and methods follow the scenario test flow pattern of spec
string definition, code generation, assertions, and build verification.
- Around line 13-324: Replace all implicit type declarations using `var` with
explicit types throughout the SettingsBuilderTests class in the test file.
Identify each variable declaration that uses `var` (such as builder, result,
config, found, bundle, exception, errors, and action variables) and replace it
with the explicit type being assigned. For example, replace `var builder = new
SettingsBuilder()` with `SettingsBuilder builder = new SettingsBuilder()`, and
similar patterns for all other local variables in the test methods to comply
with the repository's coding standards that disallow var declarations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 149228c9-b5dd-4323-b5a2-cd3d1790bf8e
📒 Files selected for processing (1)
src/Refitter.Tests/SettingsBuilderTests.cs
|
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.
Summary
Replaces the monolithic
RefitGeneratorSettingsgod object's reflection-basedConventionMapperwith explicitly-typed config slice records and aSettingsBuilderinfrastructure.Changes
New infrastructure (
src/Refitter.Core/Settings/):Slices/:GenerationConfig,OutputConfigSlice,OpenApiSourceConfigSlice,FilterConfigSlice,SchemaConfigSlice,TypeConfigSlice,ParameterConfigSlice,FeatureConfigSliceSettingsBundle— immutable bundle of keyed config slicesSettingsBuilder— fluent builder withWith<T>(),Get<T>(),TryGet<T>(),Build()SettingsResult— validation result withThrowIfInvalid()SettingsValidationException— typed validation exceptionSettingsBundleExtensions.ToLegacySettings()— backward-compatible conversion toRefitGeneratorSettingsCLI mapper refactored (
src/Refitter/):SettingsToBundleMapper.cs— new compile-time-checked explicit mapping from CLISettingsto config slicesConventionMapper.cs— deleted (replaced entirely)SettingsMapper.cs— simplified to compose bundle mapping +ToLegacySettings()+ standalone properties (CodeGeneratorSettings,ApizrSettings)Tests:
SettingsMapperTests.cs— removed 3ConventionMapper-specific tests and their helper classes (theMap_Should_Copy_Same_Name_Same_Type_Propertiestest still exercises full mapping coverage)Source Generator:
Refitter.SourceGenerator.csproj— added<Compile Include="../Refitter.Core/Settings/Slices/*.cs" />for in-process compilationDesign Decisions
Refitter.Core.Settingsnamespace to avoid name conflicts with existing mutable config classesstring[]?nullable types (consumers null-coalesce with?? [])TryGet<T>()returnsTviadefault!for netstandard2.0 compatibilitySettingsclassVerification
dotnet build -c Release)dotnet test)Summary by CodeRabbit
SettingsBuilderandSettingsBundle, along withSettingsResultandSettingsValidationExceptionfor clear validation feedback.