Strategy pattern for document loading#1165
Merged
Merged
Conversation
Introduce two narrow interfaces that separate output planning from writing: - IOutputPlanner resolves GeneratorOutput into PlannedFile paths (pure logic, no I/O) - IFileWriter accepts a PlannedFile and writes it to disk Part of PRD-003: separate output planning from writing.
Adapter wraps static OutputPlanner methods behind the IOutputPlanner interface, dispatching to PlanSingleFile or PlanMultipleFiles based on config.GenerateMultipleFiles. Tests mirror the existing OutputPlannerTests pattern but go through the interface seam.
CLI adapter writes planned files to disk with directory creation and reports progress via IGenerationReporter.ReportFileWritten(). Tests verify directory creation, existing directories, and content preservation.
MSBuild adapter writes planned files to disk with directory creation. Synchronous implementation (netstandard2.0 compat). No logging dependencies — the calling RefitterGenerateTask handles reporting. Tests verify directory creation and existing-directory scenarios.
Content-equality-aware file writer that skips writing when the file already exists with identical content, avoiding unnecessary disk writes during incremental/design-time builds. Synchronous implementation for netstandard2.0 compatibility. Tests verify: directory creation, skip-on-identical-content, and overwrite-on-different-content.
GenerationOrchestrator no longer creates directories or calls File.WriteAllTextAsync directly. It delegates path planning to IOutputPlanner (OutputPlannerAdapter) and file writing to IFileWriter (CliFileWriter). The WriteSingleFile and WriteMultipleFiles methods became instance methods to access the planner field. This is the first distribution form refactored as part of PRD-003.
ProcessRefitterFile now delegates path planning to OutputPlannerAdapter and file writing to MsBuildFileWriter instead of inline Directory.CreateDirectory + File.WriteAllText. This is the second distribution form refactored as part of PRD-003.
Replaced inline WriteGeneratedFile logic (directory creation + content-equality check + File.WriteAllText) with SourceGeneratorFileWriter via the IFileWriter interface. Added Abstractions/*.cs to the source generator's compile includes so that SourceGeneratorFileWriter (and future abstractions) are available. This is the third distribution form refactored as part of PRD-003.
System.Runtime.CompilerServices and System.Text were only used by the old WriteGeneratedFile method and are no longer needed.
Contributor
📝 WalkthroughWalkthroughThe PR introduces a strategy pattern for ChangesDocument Loading Strategy Pattern
Output Planning and File Writing Abstractions
Sequence Diagram(s)sequenceDiagram
participant DocumentLoader
participant FileDocumentStrategy
participant HttpDocumentStrategy
participant OpenApiReaderDocumentStrategy
DocumentLoader->>FileDocumentStrategy: TryLoadAsync(path)
alt returns non-null
FileDocumentStrategy-->>DocumentLoader: OpenApiDocument
else returns null
DocumentLoader->>HttpDocumentStrategy: TryLoadAsync(path)
alt returns non-null
HttpDocumentStrategy-->>DocumentLoader: OpenApiDocument
else returns null
DocumentLoader->>OpenApiReaderDocumentStrategy: TryLoadAsync(path)
alt returns non-null
OpenApiReaderDocumentStrategy-->>DocumentLoader: OpenApiDocument
else all strategies failed
DocumentLoader-->>DocumentLoader: throw InvalidOperationException
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1165 +/- ##
==========================================
- Coverage 94.81% 94.78% -0.04%
==========================================
Files 69 75 +6
Lines 3454 3568 +114
==========================================
+ Hits 3275 3382 +107
- Misses 69 76 +7
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:
|
![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: 11
🧹 Nitpick comments (12)
src/Refitter.Tests/OpenApi/OpenApiReaderDocumentStrategyTests.cs (1)
7-10: 💤 Low valueRemove extra blank line.
Lines 9-10 contain two consecutive blank lines at the start of the class. This is inconsistent with typical C# formatting conventions. Consider removing one blank line.
♻️ Proposed fix
public class OpenApiReaderDocumentStrategyTests { - [Test]🤖 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/OpenApi/OpenApiReaderDocumentStrategyTests.cs` around lines 7 - 10, The OpenApiReaderDocumentStrategyTests class has two consecutive blank lines immediately after the opening brace, which violates standard C# formatting conventions. Remove one of the blank lines so there is only a single blank line (or no blank line) between the opening brace and the class content.src/Refitter.Tests/OpenApi/PathUtilitiesTests.cs (1)
28-41: ⚡ Quick winRemove duplicate test case.
Lines 31 and 37 both contain
[Arguments("spec.YAML")], which duplicates the same test input. One of these should be removed to avoid redundant test execution.♻️ Proposed fix
[Arguments("spec.yaml")] [Arguments("spec.yml")] [Arguments("spec.YAML")] [Arguments("spec.YML")] [Arguments("/path/to/spec.yaml")] [Arguments("https://example.com/spec.yaml")] [Arguments("https://example.com/spec.yaml?query=param")] [Arguments("https://example.com/spec.yaml#fragment")] - [Arguments("spec.YAML")] public void IsYaml_Detects_Yaml_Paths(string path)🤖 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/OpenApi/PathUtilitiesTests.cs` around lines 28 - 41, The IsYaml_Detects_Yaml_Paths test method in PathUtilitiesTests class has a duplicate test argument decorator. The Arguments attribute for "spec.YAML" appears twice in the attribute list above the method. Remove one of the duplicate Arguments("spec.YAML") decorators to eliminate the redundant test case execution.src/Refitter/CliFileWriter.cs (2)
26-27: ⚡ Quick winUse explicit local type instead of
varfordir.This changed segment introduces
var, which conflicts with the configured C# style preference.Suggested change
- var dir = Path.GetDirectoryName(file.Path); + string? dir = Path.GetDirectoryName(file.Path);As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent)."🤖 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/CliFileWriter.cs` around lines 26 - 27, The variable `dir` in the CliFileWriter.cs file is declared using `var` keyword, which violates the configured C# style preference that discourages var usage. Replace the `var` keyword with the explicit type `string` for the `dir` variable assignment where Path.GetDirectoryName is called to maintain consistency with the coding guidelines.Source: Coding guidelines
10-10: ⚡ Quick winRename underscore-prefixed private field to camelCase.
_reporterviolates the repository field naming rule for private members.Suggested change
- private readonly IGenerationReporter _reporter; + private readonly IGenerationReporter reporter; @@ - _reporter = reporter ?? throw new ArgumentNullException(nameof(reporter)); + this.reporter = reporter ?? throw new ArgumentNullException(nameof(reporter)); @@ - _reporter.ReportFileWritten(file.Path); + reporter.ReportFileWritten(file.Path);As per coding guidelines, "Private fields must NOT be prefixed with underscore; use camelCase without underscore prefix (e.g.,
openApiPathnot_openApiPath)."Also applies to: 18-19, 31-31
🤖 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/CliFileWriter.cs` at line 10, Rename all underscore-prefixed private fields to camelCase without the underscore prefix to comply with the repository's coding guidelines. Specifically, change the field `_reporter` in the CliFileWriter class from `private readonly IGenerationReporter _reporter` to use camelCase without underscore. Apply this same renaming pattern to all other private fields mentioned in the comment (lines 18-19 and 31), and update all references to these fields throughout the class to use the new names without underscores.Source: Coding guidelines
src/Refitter/GenerationOrchestrator.cs (2)
10-10: ⚡ Quick winRename
_plannerto camelCase without underscore prefix.The private field naming here is inconsistent with the repository C# rule.
Suggested change
- private readonly IOutputPlanner _planner; + private readonly IOutputPlanner planner; @@ - _planner = planner ?? throw new ArgumentNullException(nameof(planner)); + this.planner = planner ?? throw new ArgumentNullException(nameof(planner));As per coding guidelines, "Private fields must NOT be prefixed with underscore; use camelCase without underscore prefix (e.g.,
openApiPathnot_openApiPath)."Also applies to: 27-27
🤖 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/GenerationOrchestrator.cs` at line 10, The private field `_planner` in the GenerationOrchestrator class violates the C# naming convention which requires private fields to use camelCase without underscore prefix. Rename the field `_planner` to `planner` throughout the class, including all references to this field. Additionally, check line 27 which has a similar naming issue and apply the same correction to any other private fields using the underscore prefix pattern.Source: Coding guidelines
61-61: ⚡ Quick winReplace
varwith explicit local types in the newly added segments.These changes introduce multiple
vardeclarations against the configured C# style preference.As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent)."Also applies to: 121-125, 131-135, 151-156
🤖 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/GenerationOrchestrator.cs` at line 61, Replace all `var` keyword declarations with their explicit types throughout the newly added segments to comply with the C# style preference that discourages use of `var`. In the CliFileWriter declaration on line 61, replace `var` with the explicit type `CliFileWriter`. Apply the same pattern to all other `var` declarations in the segments at lines 121-125, 131-135, and 151-156, replacing each `var` with the appropriate explicit type for the variable being assigned.Source: Coding guidelines
src/Refitter.MSBuild/MsBuildFileWriter.cs (1)
20-22: ⚡ Quick winReplace
varwith explicit type for directory path.Please use an explicit nullable string type for the directory local.
Suggested change
- var dir = Path.GetDirectoryName(file.Path); + string? dir = Path.GetDirectoryName(file.Path);As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent)".🤖 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.MSBuild/MsBuildFileWriter.cs` around lines 20 - 22, Replace the `var` keyword with the explicit nullable string type `string?` for the `dir` variable declaration that is assigned the result of `Path.GetDirectoryName(file.Path)`. Since `Path.GetDirectoryName()` returns a nullable string, use `string? dir = Path.GetDirectoryName(file.Path);` instead of `var dir = Path.GetDirectoryName(file.Path);` to adhere to the coding guideline that discourages the use of the `var` keyword.Source: Coding guidelines
src/Refitter.MSBuild/RefitterGenerateTask.cs (1)
77-110: ⚡ Quick winNew local declarations should use explicit types in this block.
The newly introduced locals in this section use
var; please switch to explicit types to follow project C# style.As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent)".🤖 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.MSBuild/RefitterGenerateTask.cs` around lines 77 - 110, Replace all `var` keyword declarations with explicit types throughout this method block. Specifically, change `var planner` to `OutputPlannerAdapter planner`, `var writer` to `MsBuildFileWriter writer`, `var generatedFiles` to `List<string> generatedFiles`, `var output` to `GeneratorOutput output`, `var plannedFiles` to its appropriate type (likely `List<PlannedFile>` or similar), `var code` to `string code`, and `var plannedFile` in the foreach loop to its explicit type. Ensure all local variable declarations use explicit types instead of the `var` keyword to comply with the project's C# style guidelines.Source: Coding guidelines
src/Refitter.Core/Abstractions/SourceGeneratorFileWriter.cs (1)
20-26: ⚡ Quick winUse explicit local types in this block.
Use explicit types for
dirandexistingContentto align with the C# style policy.Suggested change
- var dir = Path.GetDirectoryName(file.Path); + string? dir = Path.GetDirectoryName(file.Path); if (!string.IsNullOrWhiteSpace(dir) && !Directory.Exists(dir)) Directory.CreateDirectory(dir); - var existingContent = File.Exists(file.Path) + string? existingContent = File.Exists(file.Path) ? File.ReadAllText(file.Path, Encoding.UTF8) : null;As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent)".🤖 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/Abstractions/SourceGeneratorFileWriter.cs` around lines 20 - 26, The code uses the var keyword for local variable declarations (dir and existingContent) which violates the C# coding style policy that discourages var usage. Replace the var keyword with explicit types for both variables: change var dir to string? dir since Path.GetDirectoryName can return null, and change var existingContent to string? existingContent since it is explicitly assigned either a string from File.ReadAllText or null based on the conditional expression.Source: Coding guidelines
src/Refitter.SourceGenerator/RefitterSourceGenerator.cs (1)
102-103: ⚡ Quick winUse explicit local types for newly added declarations.
Please replace
varwith explicit types in these new declarations to align with project style.As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent)".Also applies to: 128-129
🤖 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/RefitterSourceGenerator.cs` around lines 102 - 103, Replace the `var` keyword with explicit type declarations to align with the project's coding style guidelines. In the RefitterSourceGenerator class, change `var planned = new PlannedFile(outputPath, refit);` to use the explicit type `PlannedFile` instead of `var`. Apply the same fix to any other `var` declarations in the nearby code around lines 128-129, replacing them with their corresponding explicit types.Source: Coding guidelines
src/Refitter.Core/Abstractions/OutputPlannerAdapter.cs (1)
26-27: ⚡ Quick winUse explicit type instead of
varfor local declaration.Please replace implicit typing with an explicit type for
codeto match the repository C# style rule.Suggested change
- var code = output.Files.Count > 0 + string code = output.Files.Count > 0 ? output.Files[0].Content : string.Empty;As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent)".🤖 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/Abstractions/OutputPlannerAdapter.cs` around lines 26 - 27, Replace the implicit type `var` with the explicit type for the `code` variable in the ternary expression. Determine the appropriate explicit type based on what `output.Files[0].Content` returns and the overall type of the ternary operator expression, then declare `code` with that explicit type instead of using `var` to align with the repository's C# style guidelines that discourage implicit typing.Source: Coding guidelines
src/Refitter.Tests/MsBuildFileWriterTests.cs (1)
10-62: 💤 Low valueConsider adding an explicit overwrite test for completeness.
While the existing tests cover the basic write scenarios, adding a test similar to
SourceGeneratorFileWriterTests.WriteAsync_Overwrites_When_Content_Differswould explicitly document thatMsBuildFileWriteroverwrites existing files. This would bring test coverage parity with the other file writer test suites.📝 Suggested test
[Test] public async Task WriteAsync_Overwrites_Existing_File() { var workspace = Path.Combine( AppContext.BaseDirectory, "MsBuildFileWriterTests", Guid.NewGuid().ToString("N")); try { Directory.CreateDirectory(workspace); var filePath = Path.Combine(workspace, "Output.cs"); await File.WriteAllTextAsync(filePath, "// old content"); var writer = new MsBuildFileWriter(); var planned = new PlannedFile(filePath, "// new content"); await writer.WriteAsync(planned, default); File.Exists(filePath).Should().BeTrue(); var content = await File.ReadAllTextAsync(filePath); content.Should().Be("// new content"); } finally { if (Directory.Exists(workspace)) Directory.Delete(workspace, recursive: true); } }🤖 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/MsBuildFileWriterTests.cs` around lines 10 - 62, Add a new test method to the MsBuildFileWriterTests class called WriteAsync_Overwrites_Existing_File that explicitly verifies MsBuildFileWriter overwrites existing files with different content. The test should follow the same pattern as the existing tests (WriteAsync_Creates_Directory_And_Writes_File and WriteAsync_Does_Not_Throw_When_Directory_Already_Exists): create a workspace with a test directory, pre-write an initial file with old content, use MsBuildFileWriter to write new content to the same file path via a PlannedFile, and assert that the final file content matches the new content (not the old), with proper cleanup in the finally block.
🤖 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/Document/DocumentLoader.cs`:
- Around line 45-48: The catch block currently captures all exceptions including
cancellation exceptions and adds them to the errors collection. This prevents
cancellation from being honored and can result in InvalidOperationException
being thrown instead. Modify the catch block to check if the exception is a
cancellation exception (OperationCanceledException or TaskCanceledException) and
rethrow it immediately. Only add non-cancellation exceptions to the errors
collection using the existing errors.Add() call with strategy.GetType().Name.
- Around line 14-17: Add null validation to the DocumentLoader constructor's
strategies parameter before materializing it with ToList(). Throw an
ArgumentNullException with a descriptive message (such as "strategies cannot be
null") if the strategies parameter is null, ensuring the validation occurs
before the ToList() call on the strategies enumerable.
In `@src/Refitter.Core/Document/FileDocumentStrategy.cs`:
- Around line 23-25: The bare catch block in the FileDocumentStrategy.cs file is
swallowing all exceptions including OperationCanceledException and
TaskCanceledException, which prevents proper cancellation handling. Modify the
catch block to specifically check if the caught exception is an
OperationCanceledException or TaskCanceledException and re-throw it, while only
returning null for other non-cancellation exceptions. This preserves
cancellation semantics and allows cancellation to propagate correctly to
callers.
In `@src/Refitter.Core/Document/HttpDocumentStrategy.cs`:
- Around line 46-48: Replace the broad catch block in the HttpDocumentStrategy
that swallows all exceptions and returns null with a more specific exception
handler. Instead of catching all exceptions, catch only the specific exception
types (such as HttpRequestException or other expected failures) that should
return null, and allow OperationCanceledException and TaskCanceledException to
propagate up naturally so that cancellation is properly handled rather than
being converted to a null return value that appears as an unrelated loader
failure.
- Around line 32-38: The HttpResponseMessage returned from httpClient.GetAsync
is not being disposed, which prevents connection pooling and resource release.
Wrap the response variable (returned from httpClient.GetAsync) in a using
statement to ensure the HttpResponseMessage is properly disposed after reading
the content via response.Content.ReadAsStringAsync() and calling
response.EnsureSuccessStatusCode(). This will promptly release the connection
back to the connection pool.
In `@src/Refitter.Core/Document/OpenApiReaderDocumentStrategy.cs`:
- Around line 83-85: The bare catch block in the FallbackToNSwagAsync method is
catching all exceptions including cancellation exceptions, which prevents proper
cancellation handling. Modify the catch block to specifically check for
OperationCanceledException and re-throw it, while continuing to catch and handle
other exception types by returning null. This preserves the cancellation
behavior while maintaining the fallback logic for other errors.
In `@src/Refitter.Core/Document/PathUtilities.cs`:
- Around line 24-25: The EndsWith method calls in the basePath validation are
matching incomplete extension patterns, allowing non-YAML files to be
incorrectly classified. Instead of checking EndsWith("yaml") and
EndsWith("yml"), change them to check for the dot-prefixed extensions ".yaml"
and ".yml" respectively. This ensures only actual YAML file extensions are
matched and prevents false positives from files with names ending in these
characters but without the proper extension format.
In `@src/Refitter.SourceGenerator/RefitterSourceGenerator.cs`:
- Around line 102-103: The cancellationToken parameter received by the
GenerateCode method is not being propagated to the WriteGeneratedFile method
calls or subsequently to the WriteAsync method, which allows disk I/O to
continue even after cancellation is requested. Modify the WriteGeneratedFile
method signature to accept the cancellationToken parameter, then pass the
cancellationToken from both WriteGeneratedFile method calls (at line 102-103 and
at lines 124-130) and forward it to the WriteAsync method call inside
WriteGeneratedFile to ensure cancellation is properly respected throughout the
file writing process.
In `@src/Refitter.Tests/OpenApi/OpenApiReaderDocumentStrategyTests.cs`:
- Around line 80-121: The test method Handles_Yaml_With_External_References
creates a temporary directory using Directory.CreateDirectory(folder) but never
deletes it, causing temporary files to accumulate in the system temp folder. Add
cleanup code at the end of the test to recursively delete the temporary
directory folder using Directory.Delete with the recursive parameter set to
true, ensuring it runs after all test assertions complete. This can be done by
wrapping the test logic in a try-finally block or by adding a direct cleanup
call at the end of the test method.
- Around line 26-78: The test method Handles_Json_With_External_References
creates a temporary directory stored in the folder variable at the start of the
test but does not remove it when the test completes. Add cleanup logic after the
test assertions to delete the temporary folder and its contents by calling
Directory.Delete with the recursive flag set to true, ensuring the folder
variable path is properly cleaned up to prevent accumulation of temporary files
in the system temp directory.
In `@src/Refitter.Tests/OutputPlannerAdapterTests.cs`:
- Line 9: Rename the private static readonly field `Planner` to `planner` to
comply with camelCase naming conventions for private fields. Update all
references to this field throughout the test class, including the field
declaration itself and all usages within the test methods to use the new
lowercase `planner` identifier.
---
Nitpick comments:
In `@src/Refitter.Core/Abstractions/OutputPlannerAdapter.cs`:
- Around line 26-27: Replace the implicit type `var` with the explicit type for
the `code` variable in the ternary expression. Determine the appropriate
explicit type based on what `output.Files[0].Content` returns and the overall
type of the ternary operator expression, then declare `code` with that explicit
type instead of using `var` to align with the repository's C# style guidelines
that discourage implicit typing.
In `@src/Refitter.Core/Abstractions/SourceGeneratorFileWriter.cs`:
- Around line 20-26: The code uses the var keyword for local variable
declarations (dir and existingContent) which violates the C# coding style policy
that discourages var usage. Replace the var keyword with explicit types for both
variables: change var dir to string? dir since Path.GetDirectoryName can return
null, and change var existingContent to string? existingContent since it is
explicitly assigned either a string from File.ReadAllText or null based on the
conditional expression.
In `@src/Refitter.MSBuild/MsBuildFileWriter.cs`:
- Around line 20-22: Replace the `var` keyword with the explicit nullable string
type `string?` for the `dir` variable declaration that is assigned the result of
`Path.GetDirectoryName(file.Path)`. Since `Path.GetDirectoryName()` returns a
nullable string, use `string? dir = Path.GetDirectoryName(file.Path);` instead
of `var dir = Path.GetDirectoryName(file.Path);` to adhere to the coding
guideline that discourages the use of the `var` keyword.
In `@src/Refitter.MSBuild/RefitterGenerateTask.cs`:
- Around line 77-110: Replace all `var` keyword declarations with explicit types
throughout this method block. Specifically, change `var planner` to
`OutputPlannerAdapter planner`, `var writer` to `MsBuildFileWriter writer`, `var
generatedFiles` to `List<string> generatedFiles`, `var output` to
`GeneratorOutput output`, `var plannedFiles` to its appropriate type (likely
`List<PlannedFile>` or similar), `var code` to `string code`, and `var
plannedFile` in the foreach loop to its explicit type. Ensure all local variable
declarations use explicit types instead of the `var` keyword to comply with the
project's C# style guidelines.
In `@src/Refitter.SourceGenerator/RefitterSourceGenerator.cs`:
- Around line 102-103: Replace the `var` keyword with explicit type declarations
to align with the project's coding style guidelines. In the
RefitterSourceGenerator class, change `var planned = new PlannedFile(outputPath,
refit);` to use the explicit type `PlannedFile` instead of `var`. Apply the same
fix to any other `var` declarations in the nearby code around lines 128-129,
replacing them with their corresponding explicit types.
In `@src/Refitter.Tests/MsBuildFileWriterTests.cs`:
- Around line 10-62: Add a new test method to the MsBuildFileWriterTests class
called WriteAsync_Overwrites_Existing_File that explicitly verifies
MsBuildFileWriter overwrites existing files with different content. The test
should follow the same pattern as the existing tests
(WriteAsync_Creates_Directory_And_Writes_File and
WriteAsync_Does_Not_Throw_When_Directory_Already_Exists): create a workspace
with a test directory, pre-write an initial file with old content, use
MsBuildFileWriter to write new content to the same file path via a PlannedFile,
and assert that the final file content matches the new content (not the old),
with proper cleanup in the finally block.
In `@src/Refitter.Tests/OpenApi/OpenApiReaderDocumentStrategyTests.cs`:
- Around line 7-10: The OpenApiReaderDocumentStrategyTests class has two
consecutive blank lines immediately after the opening brace, which violates
standard C# formatting conventions. Remove one of the blank lines so there is
only a single blank line (or no blank line) between the opening brace and the
class content.
In `@src/Refitter.Tests/OpenApi/PathUtilitiesTests.cs`:
- Around line 28-41: The IsYaml_Detects_Yaml_Paths test method in
PathUtilitiesTests class has a duplicate test argument decorator. The Arguments
attribute for "spec.YAML" appears twice in the attribute list above the method.
Remove one of the duplicate Arguments("spec.YAML") decorators to eliminate the
redundant test case execution.
In `@src/Refitter/CliFileWriter.cs`:
- Around line 26-27: The variable `dir` in the CliFileWriter.cs file is declared
using `var` keyword, which violates the configured C# style preference that
discourages var usage. Replace the `var` keyword with the explicit type `string`
for the `dir` variable assignment where Path.GetDirectoryName is called to
maintain consistency with the coding guidelines.
- Line 10: Rename all underscore-prefixed private fields to camelCase without
the underscore prefix to comply with the repository's coding guidelines.
Specifically, change the field `_reporter` in the CliFileWriter class from
`private readonly IGenerationReporter _reporter` to use camelCase without
underscore. Apply this same renaming pattern to all other private fields
mentioned in the comment (lines 18-19 and 31), and update all references to
these fields throughout the class to use the new names without underscores.
In `@src/Refitter/GenerationOrchestrator.cs`:
- Line 10: The private field `_planner` in the GenerationOrchestrator class
violates the C# naming convention which requires private fields to use camelCase
without underscore prefix. Rename the field `_planner` to `planner` throughout
the class, including all references to this field. Additionally, check line 27
which has a similar naming issue and apply the same correction to any other
private fields using the underscore prefix pattern.
- Line 61: Replace all `var` keyword declarations with their explicit types
throughout the newly added segments to comply with the C# style preference that
discourages use of `var`. In the CliFileWriter declaration on line 61, replace
`var` with the explicit type `CliFileWriter`. Apply the same pattern to all
other `var` declarations in the segments at lines 121-125, 131-135, and 151-156,
replacing each `var` with the appropriate explicit type for the variable being
assigned.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1939fac-8196-4caa-948f-1ddc718ecc32
📒 Files selected for processing (25)
src/Refitter.Core/Abstractions/IFileWriter.cssrc/Refitter.Core/Abstractions/IOutputPlanner.cssrc/Refitter.Core/Abstractions/OutputPlannerAdapter.cssrc/Refitter.Core/Abstractions/SourceGeneratorFileWriter.cssrc/Refitter.Core/Document/DocumentLoader.cssrc/Refitter.Core/Document/FileDocumentStrategy.cssrc/Refitter.Core/Document/HttpDocumentStrategy.cssrc/Refitter.Core/Document/IDocumentLoadingStrategy.cssrc/Refitter.Core/Document/OpenApiReaderDocumentStrategy.cssrc/Refitter.Core/Document/PathUtilities.cssrc/Refitter.MSBuild/MsBuildFileWriter.cssrc/Refitter.MSBuild/RefitterGenerateTask.cssrc/Refitter.SourceGenerator/Refitter.SourceGenerator.csprojsrc/Refitter.SourceGenerator/RefitterSourceGenerator.cssrc/Refitter.Tests/CliFileWriterTests.cssrc/Refitter.Tests/MsBuildFileWriterTests.cssrc/Refitter.Tests/OpenApi/DocumentLoaderCompositorTests.cssrc/Refitter.Tests/OpenApi/FileDocumentStrategyTests.cssrc/Refitter.Tests/OpenApi/HttpDocumentStrategyTests.cssrc/Refitter.Tests/OpenApi/OpenApiReaderDocumentStrategyTests.cssrc/Refitter.Tests/OpenApi/PathUtilitiesTests.cssrc/Refitter.Tests/OutputPlannerAdapterTests.cssrc/Refitter.Tests/SourceGeneratorFileWriterTests.cssrc/Refitter/CliFileWriter.cssrc/Refitter/GenerationOrchestrator.cs
christianhelle
changed the title
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 8 file(s) based on 11 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 8 file(s) based on 11 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
This was referenced Jul 12, 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.
Summary
Implements Strategy Pattern for Document Loading. Decomposes the monolithic \DocumentLoader\ into composable, testable strategies.
Changes
New interfaces and strategies
Utilities
Refactored
Testing
Commit History
6 micro commits:
efactor: DocumentLoader becomes compositor using strategy pattern\
Verification
Summary by CodeRabbit