Revert "Refactor Refit Generator"#1153
Closed
christianhelle wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughExtracts OpenAPI document loading, tag/path filtering, and schema cleaning from ChangesPipeline Architecture Refactor
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RefitPipeline
participant OpenApiDocumentFactory
participant RefitSchemaCleaner
participant RefitGenerator
participant RefitCodeGenerator
rect rgba(70, 130, 180, 0.5)
note over Caller,RefitPipeline: Async path (file-based)
Caller->>RefitPipeline: CreateAsync(settings)
RefitPipeline->>OpenApiDocumentFactory: CreateAsync(OpenApiPath)
OpenApiDocumentFactory-->>RefitPipeline: OpenApiDocument
end
rect rgba(100, 160, 100, 0.5)
note over RefitPipeline,RefitGenerator: Sync pipeline (shared by both paths)
RefitPipeline->>RefitPipeline: Create(document, settings)
RefitPipeline->>RefitPipeline: FilterByTags(document, settings)
RefitPipeline->>RefitPipeline: FilterByPath(document, settings)
RefitPipeline->>RefitSchemaCleaner: Clean(document, removeUnusedSchema, keepPatterns, hierarchy)
RefitSchemaCleaner-->>RefitPipeline: cleaned OpenApiDocument
RefitPipeline->>RefitGenerator: new RefitGenerator(cleanedDocument, settings)
end
Caller->>RefitGenerator: Generate() / GenerateMultipleFiles()
RefitGenerator->>RefitCodeGenerator: Generate(document, settings)
RefitCodeGenerator-->>Caller: string / GeneratorOutput
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Refitter.Tests/RefitPipelineTests.cs (1)
127-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd compile verification for generated code paths.
These tests validate content/non-empty output but do not verify that generated code actually compiles. Please add
BuildHelper.BuildCSharp()assertions (includingtargetFrameworkcoverage fornet8.0,net9.0, andnet10.0) for the generated outputs in these methods.As per coding guidelines, tests must use
BuildHelper.BuildCSharp()to verify generated code compiles, supportingnet8.0,net9.0, andnet10.0via thetargetFrameworkparameter.Also applies to: 144-160, 182-196
🤖 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.Tests/RefitPipelineTests.cs` around lines 127 - 141, The Pipeline_Generate_ProducesValidCode method generates code but only validates that the output is non-empty and contains expected strings, without verifying that the generated code actually compiles. Add BuildHelper.BuildCSharp() assertions after the generatedCode variable is set to verify compilation success, testing across three target frameworks: net8.0, net9.0, and net10.0 using the targetFramework parameter. Apply this same fix to the other similar test methods mentioned in the comment (the ones at lines 144-160 and 182-196) to ensure all generated code paths are verified to compile across all supported frameworks.Source: Coding guidelines
🧹 Nitpick comments (5)
src/Refitter.Core/RefitPipeline.cs (2)
25-26: 💤 Low valueReplace
varwith explicit types.As per coding guidelines, the use of
varkeyword is discouraged. Use explicit types instead.♻️ Suggested fix
- var processed = RefitDocumentFilter.FilterByTags(document, settings.IncludeTags); - processed = RefitDocumentFilter.FilterByPath(processed, settings.IncludePathMatches); + OpenApiDocument processed = RefitDocumentFilter.FilterByTags(document, settings.IncludeTags); + processed = RefitDocumentFilter.FilterByPath(processed, settings.IncludePathMatches);And on line 46:
- var openApiDocument = await GetOpenApiDocument(settings).ConfigureAwait(false); + OpenApiDocument openApiDocument = await GetOpenApiDocument(settings).ConfigureAwait(false);Also applies to: 46-46
🤖 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/RefitPipeline.cs` around lines 25 - 26, Replace the `var` keyword with explicit types in the RefitPipeline class. For the `processed` variable assignments that result from calling RefitDocumentFilter.FilterByTags() and RefitDocumentFilter.FilterByPath(), declare the variable with its explicit return type instead of using `var`. Identify the actual return type from these filter methods and use that type annotation for the variable declaration.Source: Coding guidelines
11-11: ⚡ Quick winPrivate static fields should use camelCase per coding guidelines.
Both files define private static readonly fields using PascalCase, which violates the guideline requiring camelCase without underscore prefix for private fields.
src/Refitter.Core/RefitPipeline.cs#L11-L11: RenameSchemaCleanertoschemaCleanerand update references on lines 27-31.src/Refitter.Core/RefitGenerator.cs#L11-L11: RenameCodeGeneratortocodeGeneratorand update references on lines 36 and 42.🤖 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/RefitPipeline.cs` at line 11, Private static fields must follow camelCase naming convention without underscore prefix. In src/Refitter.Core/RefitPipeline.cs at line 11, rename the static field SchemaCleaner to schemaCleaner and update all references to this field on lines 27-31 to use the new camelCase name. In src/Refitter.Core/RefitGenerator.cs at line 11, rename the static field CodeGenerator to codeGenerator and update all references to this field on lines 36 and 42 to use the new camelCase name.Source: Coding guidelines
src/Refitter.Core/RefitSchemaCleaner.cs (1)
31-32: ⚡ Quick winUse explicit local types instead of
varin core cleaner logic.Please replace
varwith explicit types forresultandcleanerto match repository C# style guidance.As per coding guidelines, "
**/*.cs: Discourage the use ofvarkeyword; use explicit types instead."🤖 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/RefitSchemaCleaner.cs` around lines 31 - 32, Replace the `var` keyword with explicit types in the RefitSchemaCleaner.cs file to comply with repository C# style guidance. For the `result` variable assigned from the CloneDocument method call, use the explicit return type of CloneDocument. For the `cleaner` variable assigned from the SchemaCleaner constructor, use the explicit type SchemaCleaner instead of var for both declarations.Source: Coding guidelines
src/Refitter.Tests/RefitSchemaCleanerTests.cs (1)
36-40: ⚡ Quick winTests follow TUnit and OpenAPI 2.0/3.0 coverage, but local typing should be explicit.
Please replace repeated
vardeclarations with explicit types in this test class to conform to project C# conventions.As per coding guidelines, "
**/*.cs: Discourage the use ofvarkeyword; use explicit types instead."Also applies to: 75-80, 115-121, 159-164, 197-202, 235-240, 274-280, 317-322
🤖 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.Tests/RefitSchemaCleanerTests.cs` around lines 36 - 40, Replace all var keyword declarations with explicit types throughout the RefitSchemaCleanerTests.cs test class to conform to project C# conventions. In the test method shown, replace var with explicit types for variables like json (OpenApiYamlDocument), document (OpenApiDocument), sut (RefitSchemaCleaner), and result (infer the return type from the Clean method). Apply the same pattern consistently across all test methods in this class, declaring variables with their actual types instead of using var.Source: Coding guidelines
src/Refitter.Tests/RefitPipelineTests.cs (1)
82-82: ⚡ Quick winReplace
varwith explicit types in changed declarations.
varis used in multiple changed lines (for example, Line 82 and Line 136). Please switch these to explicit types for consistency with repository C# conventions.As per coding guidelines,
varis discouraged in**/*.csfiles and explicit types should be used.Also applies to: 94-94, 101-101, 113-114, 136-136, 156-156, 173-173, 191-191
🤖 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.Tests/RefitPipelineTests.cs` at line 82, Replace all instances of `var` declarations with explicit types in the RefitPipelineTests.cs file to comply with repository C# conventions. Specifically, identify each `var` keyword used in variable declarations (such as the `sut` variable assigned from RefitPipeline.CreateAsync() at the anchor location, and the other locations mentioned in the comment) and replace it with the appropriate explicit type. Determine the correct type for each variable by examining its assignment and use the fully qualified type name where necessary.Source: Coding guidelines
🤖 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/IRefitSchemaCleaner.cs`:
- Around line 5-8: The documentation for the Clean method in IRefitSchemaCleaner
interface claims to return a new document, but the actual implementation in
RefitSchemaCleaner.Clean returns the original document instance when
removeUnusedSchema is false. Update the XML documentation summary comment at
IRefitSchemaCleaner (lines 5-8) to clarify that the method returns a
non-mutating result that does not modify the input document, rather than
guaranteeing a new instance. Apply the same documentation update to the
RefitSchemaCleaner.Clean method implementation (lines 19-23) to maintain
consistency between the interface contract and implementation.
---
Outside diff comments:
In `@src/Refitter.Tests/RefitPipelineTests.cs`:
- Around line 127-141: The Pipeline_Generate_ProducesValidCode method generates
code but only validates that the output is non-empty and contains expected
strings, without verifying that the generated code actually compiles. Add
BuildHelper.BuildCSharp() assertions after the generatedCode variable is set to
verify compilation success, testing across three target frameworks: net8.0,
net9.0, and net10.0 using the targetFramework parameter. Apply this same fix to
the other similar test methods mentioned in the comment (the ones at lines
144-160 and 182-196) to ensure all generated code paths are verified to compile
across all supported frameworks.
---
Nitpick comments:
In `@src/Refitter.Core/RefitPipeline.cs`:
- Around line 25-26: Replace the `var` keyword with explicit types in the
RefitPipeline class. For the `processed` variable assignments that result from
calling RefitDocumentFilter.FilterByTags() and
RefitDocumentFilter.FilterByPath(), declare the variable with its explicit
return type instead of using `var`. Identify the actual return type from these
filter methods and use that type annotation for the variable declaration.
- Line 11: Private static fields must follow camelCase naming convention without
underscore prefix. In src/Refitter.Core/RefitPipeline.cs at line 11, rename the
static field SchemaCleaner to schemaCleaner and update all references to this
field on lines 27-31 to use the new camelCase name. In
src/Refitter.Core/RefitGenerator.cs at line 11, rename the static field
CodeGenerator to codeGenerator and update all references to this field on lines
36 and 42 to use the new camelCase name.
In `@src/Refitter.Core/RefitSchemaCleaner.cs`:
- Around line 31-32: Replace the `var` keyword with explicit types in the
RefitSchemaCleaner.cs file to comply with repository C# style guidance. For the
`result` variable assigned from the CloneDocument method call, use the explicit
return type of CloneDocument. For the `cleaner` variable assigned from the
SchemaCleaner constructor, use the explicit type SchemaCleaner instead of var
for both declarations.
In `@src/Refitter.Tests/RefitPipelineTests.cs`:
- Line 82: Replace all instances of `var` declarations with explicit types in
the RefitPipelineTests.cs file to comply with repository C# conventions.
Specifically, identify each `var` keyword used in variable declarations (such as
the `sut` variable assigned from RefitPipeline.CreateAsync() at the anchor
location, and the other locations mentioned in the comment) and replace it with
the appropriate explicit type. Determine the correct type for each variable by
examining its assignment and use the fully qualified type name where necessary.
In `@src/Refitter.Tests/RefitSchemaCleanerTests.cs`:
- Around line 36-40: Replace all var keyword declarations with explicit types
throughout the RefitSchemaCleanerTests.cs test class to conform to project C#
conventions. In the test method shown, replace var with explicit types for
variables like json (OpenApiYamlDocument), document (OpenApiDocument), sut
(RefitSchemaCleaner), and result (infer the return type from the Clean method).
Apply the same pattern consistently across all test methods in this class,
declaring variables with their actual types instead of using var.
🪄 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: bba9cb40-4ccd-405d-b996-79e346e4d286
📒 Files selected for processing (8)
src/Refitter.Core/IRefitCodeGenerator.cssrc/Refitter.Core/IRefitSchemaCleaner.cssrc/Refitter.Core/RefitCodeGenerator.cssrc/Refitter.Core/RefitGenerator.cssrc/Refitter.Core/RefitPipeline.cssrc/Refitter.Core/RefitSchemaCleaner.cssrc/Refitter.Tests/RefitPipelineTests.cssrc/Refitter.Tests/RefitSchemaCleanerTests.cs
Comment on lines
+5
to
+8
| /// <summary> | ||
| /// Cleans an OpenAPI document by removing unreferenced schemas. | ||
| /// Returns a new document without mutating the input. | ||
| /// </summary> |
Contributor
There was a problem hiding this comment.
Public contract says “new document,” but implementation can return the original instance.
The interface documentation currently guarantees a new instance, while RefitSchemaCleaner.Clean returns the original document when removeUnusedSchema is false. Please align this contract to “non-mutating” semantics (or change implementation to always clone).
Also applies to: 19-23
🤖 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/IRefitSchemaCleaner.cs` around lines 5 - 8, The
documentation for the Clean method in IRefitSchemaCleaner interface claims to
return a new document, but the actual implementation in RefitSchemaCleaner.Clean
returns the original document instance when removeUnusedSchema is false. Update
the XML documentation summary comment at IRefitSchemaCleaner (lines 5-8) to
clarify that the method returns a non-mutating result that does not modify the
input document, rather than guaranteeing a new instance. Apply the same
documentation update to the RefitSchemaCleaner.Clean method implementation
(lines 19-23) to maintain consistency between the interface contract and
implementation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1153 +/- ##
==========================================
- Coverage 94.57% 94.54% -0.03%
==========================================
Files 65 67 +2
Lines 3095 3100 +5
==========================================
+ Hits 2927 2931 +4
- Misses 58 59 +1
Partials 110 110
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:
|
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.
Reverts #1151
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests