Update GeneratorPipeline to use injected IContractsPostProcessor adapters#1139
Conversation
Add IContractsPostProcessor interface for post-generation contract transformation. Create Swagger2OptionalReferenceNullabilityNormalizer and EnumStringConverterInjector implementations, extracted from GeneratorPipeline.SanitizeGeneratedContracts and NormalizeSwagger2OptionalReferencePropertyNullability.
GeneratorPipeline now accepts XmlDocumentationGenerator, InterfaceGenerator, and IContractsPostProcessor collection via constructor instead of creating them inline. The three internal static helpers (SanitizeGeneratedContracts, NormalizeSwagger2OptionalReferencePropertyNullability, GenerateJsonSerializerContext) are removed from public API surface. SanitizeGeneratedContracts and NormalizeSwagger2OptionalReferencePropertyNullability now live in the IContractsPostProcessor implementations. GenerateJsonSerializerContext remains as a private helper called inline in Run().
Removed private wrapper methods (SanitizeGeneratedContracts, NormalizeSwagger2OptionalReferencePropertyNullability, GenerateJsonSerializerContext) that leaked pipeline internals. RunPipeline() now constructs GeneratorPipeline with real XmlDocumentationGenerator, InterfaceGenerator, and post-processors.
Update GeneratorPipelineTests to construct GeneratorPipeline with constructor args. Update RefitGeneratorAdvancedTests to test Swagger2OptionalReferenceNullabilityNormalizer and JsonSerializerContextGenerator directly instead of using reflection on removed private methods.
Test was using reflection on removed RefitGenerator.SanitizeGeneratedContracts method. Now tests EnumStringConverterInjector directly.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces an ChangesContracts Post-Processor Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Refitter.Core/GeneratorPipeline.cs (1)
11-18: ⚡ Quick winRemove unused
docGeneratorparameter.The
docGeneratorparameter in theGeneratorPipelineconstructor is not stored or used anywhere in the class. SinceInterfaceGeneratoris already constructed withdocGenerator(as seen inRefitGenerator.RunPipelineat line 166), there's no need to pass it again toGeneratorPipeline.♻️ Proposed fix
internal GeneratorPipeline( - XmlDocumentationGenerator docGenerator, InterfaceGenerator interfaceGenerator, IEnumerable<IContractsPostProcessor> contractsPostProcessors)Also update the call site in
RefitGenerator.csline 168:var pipeline = new GeneratorPipeline( - docGenerator, interfaceGenerator,And update
GeneratorPipelineTests.csline 136:var pipeline = new GeneratorPipeline( - docGenerator, interfaceGenerator,🤖 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/GeneratorPipeline.cs` around lines 11 - 18, The docGenerator parameter in the GeneratorPipeline constructor is unused and should be removed since InterfaceGenerator is already constructed with the docGenerator elsewhere. Remove the docGenerator parameter from the GeneratorPipeline constructor signature in src/Refitter.Core/GeneratorPipeline.cs (lines 11-18). Update the constructor call in RefitGenerator.cs line 168 to not pass the docGenerator argument. Update the GeneratorPipeline instantiation in GeneratorPipelineTests.cs line 136 to remove the docGenerator argument from the constructor call.src/Refitter.Core/IContractsPostProcessor.cs (1)
2-2: 💤 Low valueRemove redundant using directive.
The
using Refitter.Core;directive on line 2 is redundant since this file already declaresnamespace Refitter.Core;on line 4.♻️ Proposed fix
using NSwag; -using Refitter.Core; namespace Refitter.Core;🤖 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/IContractsPostProcessor.cs` at line 2, Remove the redundant using Refitter.Core directive since the file is already declared within the namespace Refitter.Core. Delete the using statement and keep only the namespace declaration, as a file does not need to import the namespace it belongs to.
🤖 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/Swagger2OptionalReferenceNullabilityNormalizer.cs`:
- Around line 39-49: The `IsReferenceType` method assumes all
`IdentifierNameSyntax`, `GenericNameSyntax`, `QualifiedNameSyntax`, and
`AliasQualifiedNameSyntax` nodes are reference types, but this heuristic may
incorrectly handle custom struct types. Add test case coverage for optional
struct properties (such as `MyStruct?`) to document and verify the normalizer's
behavior when custom value types appear as optional properties, explicitly
capturing whether the normalizer correctly preserves or incorrectly unwraps
nullable semantics for struct types. This test should complement the existing
coverage for arrays, generic types, and qualified names.
---
Nitpick comments:
In `@src/Refitter.Core/GeneratorPipeline.cs`:
- Around line 11-18: The docGenerator parameter in the GeneratorPipeline
constructor is unused and should be removed since InterfaceGenerator is already
constructed with the docGenerator elsewhere. Remove the docGenerator parameter
from the GeneratorPipeline constructor signature in
src/Refitter.Core/GeneratorPipeline.cs (lines 11-18). Update the constructor
call in RefitGenerator.cs line 168 to not pass the docGenerator argument. Update
the GeneratorPipeline instantiation in GeneratorPipelineTests.cs line 136 to
remove the docGenerator argument from the constructor call.
In `@src/Refitter.Core/IContractsPostProcessor.cs`:
- Line 2: Remove the redundant using Refitter.Core directive since the file is
already declared within the namespace Refitter.Core. Delete the using statement
and keep only the namespace declaration, as a file does not need to import the
namespace it belongs to.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d1b1707-0295-4f8f-b5b6-91ffe07f1fbb
📒 Files selected for processing (8)
src/Refitter.Core/EnumStringConverterInjector.cssrc/Refitter.Core/GeneratorPipeline.cssrc/Refitter.Core/IContractsPostProcessor.cssrc/Refitter.Core/RefitGenerator.cssrc/Refitter.Core/Swagger2OptionalReferenceNullabilityNormalizer.cssrc/Refitter.Tests/GeneratorPipelineTests.cssrc/Refitter.Tests/Regression/RefitGeneratorAdvancedTests.cssrc/Refitter.Tests/Scenarios/InlineJsonConvertersTests.cs
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|



Changes
ew\
Verification
Summary by CodeRabbit