Separate output planning from writing#1164
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
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughIntroduces ChangesOutput Planning and File-Writing Abstraction
Sequence Diagram(s)sequenceDiagram
participant Caller
participant GenerationOrchestrator
participant OutputPlannerAdapter
participant CliFileWriter
participant IGenerationReporter
Caller->>GenerationOrchestrator: RunAsync(settings, reporter, ct)
GenerationOrchestrator->>GenerationOrchestrator: new CliFileWriter(reporter)
GenerationOrchestrator->>OutputPlannerAdapter: Plan(output, config, settingsFilePath, cliOutputPath)
OutputPlannerAdapter-->>GenerationOrchestrator: IReadOnlyList<PlannedFile>
loop each PlannedFile
GenerationOrchestrator->>CliFileWriter: WriteAsync(plannedFile, ct)
CliFileWriter->>CliFileWriter: Directory.CreateDirectory
CliFileWriter->>CliFileWriter: File.WriteAllTextAsync
CliFileWriter->>IGenerationReporter: ReportFileWritten(path)
end
GenerationOrchestrator-->>Caller: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 #1164 +/- ##
==========================================
- Coverage 94.81% 94.68% -0.14%
==========================================
Files 69 71 +2
Lines 3454 3497 +43
==========================================
+ Hits 3275 3311 +36
- Misses 69 74 +5
- Partials 110 112 +2
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: 4
🧹 Nitpick comments (9)
src/Refitter/CliFileWriter.cs (2)
10-10: ⚡ Quick winRename the private field to camelCase without underscore.
_reporteron Line 10 violates the repository field naming rule.As per coding guidelines, "Private fields must NOT be prefixed with underscore; use camelCase without underscore prefix (e.g.,
openApiPathnot_openApiPath)."🤖 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, The private field `_reporter` in the CliFileWriter class violates the repository's naming convention which requires private fields to use camelCase without underscore prefix. Rename the field declaration `_reporter` to `reporter` and update all references to this field throughout the CliFileWriter class to use the new name without the underscore prefix.Source: Coding guidelines
26-26: ⚡ Quick winUse an explicit local type instead of
var.The
varlocal on Line 26 conflicts with the configured C# style in this repository.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` at line 26, Replace the `var` keyword on line 26 where the `dir` variable is declared with its explicit type `string`. The variable `dir` is assigned the result of Path.GetDirectoryName which returns a string, so change `var dir = Path.GetDirectoryName(file.Path);` to use the explicit type declaration instead of the implicit var keyword to comply with the repository's C# style guidelines that discourage use of var.Source: Coding guidelines
src/Refitter/GenerationOrchestrator.cs (2)
10-10: ⚡ Quick winRename the private field to remove underscore prefix.
_planneron Line 10 violates the repository’s private-field naming rule.As per coding guidelines, "Private fields must NOT be prefixed with underscore; use camelCase without underscore prefix (e.g.,
openApiPathnot_openApiPath)."🤖 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 violates the repository's naming conventions by using an underscore prefix. Rename this field from _planner to planner (using camelCase without the underscore prefix) in the GenerationOrchestrator class. Ensure all references to this field throughout the class are updated to use the new name without the underscore.Source: Coding guidelines
61-65: ⚡ Quick winUse explicit local types instead of
varin the new locals.The changed locals in these ranges use
var, which conflicts with repository C# style.As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent)."Also applies to: 121-136, 151-159
🤖 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` around lines 61 - 65, Replace the `var` keyword with explicit types in local variable declarations throughout the file to comply with repository C# style guidelines. Specifically, change `var writer = new CliFileWriter(reporter)` to use the explicit type `CliFileWriter` instead of `var`, and apply the same pattern to any other `var` declarations found in the line ranges mentioned (121-136, 151-159). Always use the explicit type name on the right side of the assignment for all local variable declarations rather than relying on type inference with `var`.Source: Coding guidelines
src/Refitter.Tests/SourceGeneratorFileWriterTests.cs (1)
12-22: ⚡ Quick winUse explicit local types instead of
varin test locals.The new test locals use
var, which conflicts with repository C# style.As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent)."Also applies to: 39-52, 71-84
🤖 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/SourceGeneratorFileWriterTests.cs` around lines 12 - 22, Replace all `var` keyword declarations with explicit types in the test method locals throughout the SourceGeneratorFileWriterTests class to comply with repository C# style guidelines. In the shown diff block, replace `var` with the explicit types for workspace (string), writer (SourceGeneratorFileWriter), filePath (string), and planned (PlannedFile). Also apply the same change to the additional instances mentioned in lines 39-52 and 71-84 to ensure consistency across the entire test file.Source: Coding guidelines
src/Refitter.Tests/CliFileWriterTests.cs (2)
13-17: ⚡ Quick winUse explicit local types instead of
varin test locals.The new test code uses
varwidely, which conflicts with repository C# style.As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent)."Also applies to: 20-24, 41-44, 49-56, 74-77, 82-86
🤖 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/CliFileWriterTests.cs` around lines 13 - 17, Replace all instances of the `var` keyword with explicit type declarations in the test locals throughout the CliFileWriterTests.cs file to comply with the repository's C# style guidelines. Specifically, change `var workspace` to use the explicit `string` type (since Path.Combine returns a string), and apply the same pattern to all other local variables currently declared with `var` across the lines mentioned (20-24, 41-44, 49-56, 74-77, 82-86). Each variable declaration should use its actual return type instead of relying on type inference.Source: Coding guidelines
20-30: ⚡ Quick winAdd an assertion that
ReportFileWrittenis invoked.These tests validate disk output, but they don’t validate the CLI-specific reporting behavior that this adapter adds. Please assert reporter interaction with a spy/mock reporter.
Also applies to: 52-63, 83-92
🤖 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/CliFileWriterTests.cs` around lines 20 - 30, The test is missing an assertion to verify that the reporter's ReportFileWritten method is being invoked by the CliFileWriter. Replace the SimpleGenerationReporter instance with a mock or spy reporter that allows verification of method calls, then add an assertion after the WriteAsync call to verify that ReportFileWritten was invoked with the expected file path parameter. This ensures the CLI-specific reporting behavior is validated in addition to the file output. Apply the same fix to the other affected test methods referenced in the comment.src/Refitter.Tests/MsBuildFileWriterTests.cs (2)
37-56: ⚡ Quick winStrengthen the existing-directory test by asserting written content.
WriteAsync_Does_Not_Throw_When_Directory_Already_Existscurrently only checks file existence. Asserting content here would catch accidental no-op/partial-write regressions.🤖 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 37 - 56, The test WriteAsync_Does_Not_Throw_When_Directory_Already_Exists only verifies that the file exists after calling writer.WriteAsync, but does not verify that the content was actually written correctly. Add an assertion after the File.Exists check that reads the file content and asserts it matches the content provided to the PlannedFile constructor (which is "// test"). This will catch regressions where the file is created but left empty or with partial/incorrect content.
13-17: ⚡ Quick winUse explicit local types instead of
varin test locals.The changed test locals use
var, which conflicts with repository C# style.As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent)."Also applies to: 20-23, 40-43, 49-52
🤖 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 13 - 17, Replace the `var` keyword with explicit types in test local variables to align with repository C# style guidelines that discourage the use of `var`. For the workspace variable declaration that uses Path.Combine, replace `var` with `string` since Path.Combine returns a string type. Apply this same change to all other test local variables using `var` throughout the MsBuildFileWriterTests class to maintain consistent coding style.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.SourceGenerator/RefitterSourceGenerator.cs`:
- Around line 102-103: The GenerateCode method receives a cancellationToken
parameter but does not pass it to the WriteGeneratedFile method calls (at lines
102-103 and 124-130), causing filesystem I/O operations to proceed even when the
source generation is canceled. Add a cancellationToken parameter to the
WriteGeneratedFile method signature, pass the cancellationToken argument from
GenerateCode to all WriteGeneratedFile calls, and forward that token to the
WriteAsync method invocation inside WriteGeneratedFile to properly respect
cancellation requests.
In `@src/Refitter.Tests/SourceGeneratorFileWriterTests.cs`:
- Around line 37-60: The test WriteAsync_Skips_Write_When_Content_Identical
currently only verifies the final file content matches expectations, but this
would pass even if the file was rewritten with identical text. To properly
assert that no write occurred, capture the file's last-write timestamp (using
File.GetLastWriteTimeUtc) immediately after the initial File.WriteAllTextAsync
call on filePath, then after calling writer.WriteAsync, compare the timestamp
again and assert that it remains unchanged to verify the skip-write optimization
actually worked.
In `@src/Refitter/GenerationOrchestrator.cs`:
- Around line 125-132: The code accesses planned[0] immediately after the
_planner.Plan() call without validating that the result contains any elements,
and similarly accesses planned[i] in a loop around line 155-169 without guards.
Add validation checks after the _planner.Plan() call to ensure the planned
collection is not null and contains at least one element before attempting to
access planned[0], and verify the collection has sufficient elements before
iterating over it with planned[i] in the loop.
- Around line 134-136: The file size and line count metrics in
GenerationOrchestrator.cs are being calculated from the `code` variable, but the
actual content being persisted is `plannedFile.Content`. If content
transformation occurs between these two, the reported metrics become inaccurate.
Update the FormatFileSize and lines calculation to use `plannedFile.Content`
instead of `code` to ensure the metrics reflect what is actually written to the
file.
---
Nitpick comments:
In `@src/Refitter.Tests/CliFileWriterTests.cs`:
- Around line 13-17: Replace all instances of the `var` keyword with explicit
type declarations in the test locals throughout the CliFileWriterTests.cs file
to comply with the repository's C# style guidelines. Specifically, change `var
workspace` to use the explicit `string` type (since Path.Combine returns a
string), and apply the same pattern to all other local variables currently
declared with `var` across the lines mentioned (20-24, 41-44, 49-56, 74-77,
82-86). Each variable declaration should use its actual return type instead of
relying on type inference.
- Around line 20-30: The test is missing an assertion to verify that the
reporter's ReportFileWritten method is being invoked by the CliFileWriter.
Replace the SimpleGenerationReporter instance with a mock or spy reporter that
allows verification of method calls, then add an assertion after the WriteAsync
call to verify that ReportFileWritten was invoked with the expected file path
parameter. This ensures the CLI-specific reporting behavior is validated in
addition to the file output. Apply the same fix to the other affected test
methods referenced in the comment.
In `@src/Refitter.Tests/MsBuildFileWriterTests.cs`:
- Around line 37-56: The test
WriteAsync_Does_Not_Throw_When_Directory_Already_Exists only verifies that the
file exists after calling writer.WriteAsync, but does not verify that the
content was actually written correctly. Add an assertion after the File.Exists
check that reads the file content and asserts it matches the content provided to
the PlannedFile constructor (which is "// test"). This will catch regressions
where the file is created but left empty or with partial/incorrect content.
- Around line 13-17: Replace the `var` keyword with explicit types in test local
variables to align with repository C# style guidelines that discourage the use
of `var`. For the workspace variable declaration that uses Path.Combine, replace
`var` with `string` since Path.Combine returns a string type. Apply this same
change to all other test local variables using `var` throughout the
MsBuildFileWriterTests class to maintain consistent coding style.
In `@src/Refitter.Tests/SourceGeneratorFileWriterTests.cs`:
- Around line 12-22: Replace all `var` keyword declarations with explicit types
in the test method locals throughout the SourceGeneratorFileWriterTests class to
comply with repository C# style guidelines. In the shown diff block, replace
`var` with the explicit types for workspace (string), writer
(SourceGeneratorFileWriter), filePath (string), and planned (PlannedFile). Also
apply the same change to the additional instances mentioned in lines 39-52 and
71-84 to ensure consistency across the entire test file.
In `@src/Refitter/CliFileWriter.cs`:
- Line 10: The private field `_reporter` in the CliFileWriter class violates the
repository's naming convention which requires private fields to use camelCase
without underscore prefix. Rename the field declaration `_reporter` to
`reporter` and update all references to this field throughout the CliFileWriter
class to use the new name without the underscore prefix.
- Line 26: Replace the `var` keyword on line 26 where the `dir` variable is
declared with its explicit type `string`. The variable `dir` is assigned the
result of Path.GetDirectoryName which returns a string, so change `var dir =
Path.GetDirectoryName(file.Path);` to use the explicit type declaration instead
of the implicit var keyword to comply with the repository's C# style guidelines
that discourage use of var.
In `@src/Refitter/GenerationOrchestrator.cs`:
- Line 10: The private field _planner violates the repository's naming
conventions by using an underscore prefix. Rename this field from _planner to
planner (using camelCase without the underscore prefix) in the
GenerationOrchestrator class. Ensure all references to this field throughout the
class are updated to use the new name without the underscore.
- Around line 61-65: Replace the `var` keyword with explicit types in local
variable declarations throughout the file to comply with repository C# style
guidelines. Specifically, change `var writer = new CliFileWriter(reporter)` to
use the explicit type `CliFileWriter` instead of `var`, and apply the same
pattern to any other `var` declarations found in the line ranges mentioned
(121-136, 151-159). Always use the explicit type name on the right side of the
assignment for all local variable declarations rather than relying on type
inference with `var`.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48adf978-11f2-4fd3-a458-a61b8eba28ba
📒 Files selected for processing (14)
src/Refitter.Core/Abstractions/IFileWriter.cssrc/Refitter.Core/Abstractions/IOutputPlanner.cssrc/Refitter.Core/Abstractions/OutputPlannerAdapter.cssrc/Refitter.Core/Abstractions/SourceGeneratorFileWriter.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/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 3 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 4 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 PRD-003: separates output planning from writing by introducing two narrow interfaces in Refitter.Core, each with distribution-form-specific adapters.
Changes
New interfaces (Refitter.Core)
IOutputPlanner— resolvesGeneratorOutputinto a list ofPlannedFile(path + content). Pure logic, no I/O.IFileWriter— accepts aPlannedFileand writes it to disk, creating directories as needed.Adapters
OutputPlannerAdapter(Refitter.Core) — wraps staticOutputPlannermethods behindIOutputPlannerCliFileWriter(Refitter) — writes files and reports progress viaIGenerationReporterMsBuildFileWriter(Refitter.MSBuild) — writes files synchronously (netstandard2.0)SourceGeneratorFileWriter(Refitter.Core) — writes files with content-equality check (skips when content unchanged)Refactored distribution forms
GenerationOrchestrator(CLI) — delegates planning toIOutputPlannerand writing toIFileWriter; no longer creates directories or callsFile.WriteAllTextAsyncdirectlyRefitterGenerateTask(MSBuild) — delegates toOutputPlannerAdapterandMsBuildFileWriterRefitterSourceGenerator(Source Generator) — delegates writing toSourceGeneratorFileWriterTests
OutputPlannerAdapterTests— 6 tests covering single/multi-file planning throughIOutputPlannerCliFileWriterTests— 3 tests for directory creation and content preservationMsBuildFileWriterTests— 2 tests for synchronous file writingSourceGeneratorFileWriterTests— 3 tests including content-equality skip logicCommit history (9 micro-commits)
Each change is small, logical, and independently reviewed:
IOutputPlannerandIFileWriterinterfacesOutputPlannerAdapterimplementingIOutputPlannerCliFileWriterimplementingIFileWriterMsBuildFileWriterimplementingIFileWriterSourceGeneratorFileWriterimplementingIFileWriterGenerationOrchestratorto use interfacesRefitterGenerateTaskto use interfacesRefitterSourceGeneratorto use interfacesCloses PRD-003
Summary by CodeRabbit