Decouple CLI binary dependency from MSBuild task#1154
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR decouples ChangesMSBuild In-Process Decoupling
sequenceDiagram
rect rgba(173, 216, 230, 0.5)
note over RefitterGenerateTask,OpenApiValidator: New in-process flow
end
participant RefitterGenerateTask
participant RefitterSettingsLoader
participant RefitGenerator
participant OutputPlanner
participant OpenApiValidator
RefitterGenerateTask->>RefitterSettingsLoader: LoadSettings(refitterFilePath)
RefitterSettingsLoader-->>RefitterGenerateTask: RefitGeneratorSettings
RefitterGenerateTask->>RefitterSettingsLoader: ApplyDefaults(settingsFilePath, settings)
RefitterGenerateTask->>RefitGenerator: CreateAsync(settings)
RefitGenerator-->>RefitterGenerateTask: generator
RefitterGenerateTask->>RefitGenerator: GenerateAsync() / GenerateMultipleAsync()
RefitGenerator-->>RefitterGenerateTask: code / GeneratorOutput
RefitterGenerateTask->>OutputPlanner: PlanSingleFile / PlanMultipleFiles
OutputPlanner-->>RefitterGenerateTask: PlannedFile[]
RefitterGenerateTask->>RefitterGenerateTask: File.WriteAllText(plannedPath, content)
RefitterGenerateTask->>OpenApiValidator: Validate(openApiPath)
OpenApiValidator-->>RefitterGenerateTask: OpenApiValidationResult
RefitterGenerateTask-->>RefitterGenerateTask: ITaskItem[] GeneratedFiles
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Refitter.MSBuild/Refitter.MSBuild.csproj (1)
13-27:⚠️ Potential issue | 🔴 CriticalAdd
TreatWarningsAsErrorsto the PropertyGroup.Per coding guidelines, production projects in
src/**/*.csprojmust enableTreatWarningsAsErrors. This is already set inRefitter.Core.csprojbut missing from this file. Add the following line to the<PropertyGroup>section:<TreatWarningsAsErrors>true</TreatWarningsAsErrors>🤖 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/Refitter.MSBuild.csproj` around lines 13 - 27, The PropertyGroup in the Refitter.MSBuild.csproj file is missing the TreatWarningsAsErrors setting that is required per coding guidelines and already present in Refitter.Core.csproj. Add a new line containing <TreatWarningsAsErrors>true</TreatWarningsAsErrors> to the PropertyGroup section, placing it alongside the existing SuppressDependenciesWhenPacking setting to ensure the project treats all build warnings as errors.src/Refitter.Tests/MSBuild/RefitterGenerateTaskTests.cs (1)
232-262:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest name and logic are inconsistent.
The test is named
Execute_Should_Skip_Validation_When_SkipValidation_Is_True, suggesting it should verify that OpenAPI validation is skipped and generation succeeds. However, the test writes unparseable JSON ("not-valid-json"at line 239) and expectsExecute()to returnfalse(line 256).This doesn't test whether validation is skipped; it tests that unparseable input causes failure regardless of the
SkipValidationsetting. The RefitGenerator will fail to parse the invalid JSON before OpenAPI validation even runs.To properly test validation skipping, the test should:
- Write valid JSON representing an OpenAPI document that would fail OpenAPI schema validation (e.g., missing required OpenAPI fields but still parseable as JSON).
- Set
SkipValidation = true.- Expect
Execute()to returntruebecause validation is skipped and the document is parseable.Alternatively, rename the test to reflect what it actually tests, such as
Execute_Should_Fail_On_Unparseable_OpenAPI_Even_When_SkipValidation_Is_True.🔧 Option 1: Fix the test logic to actually test validation skipping
[Test] public void Execute_Should_Skip_Validation_When_SkipValidation_Is_True() { var workspace = CreateWorkspace(); try { var openApiPath = Path.Combine(workspace, "petstore.json"); - File.WriteAllText(openApiPath, "not-valid-json"); + // Valid JSON but missing required OpenAPI fields like "info" or "paths" + File.WriteAllText(openApiPath, """{"openapi": "3.0.0"}"""); var settingsPath = Path.Combine(workspace, "petstore.refitter"); File.WriteAllText( settingsPath, """{"openApiPath": "petstore.json", "namespace": "Test"}"""); var task = new RefitterGenerateTask { BuildEngine = new RecordingBuildEngine(), ProjectFileDirectory = workspace, IncludePatterns = "petstore.refitter", SkipValidation = true }; var result = task.Execute(); - result.Should().BeFalse(); + // With SkipValidation = true and parseable (but invalid) OpenAPI, should succeed + result.Should().BeTrue(); } finally { DeleteWorkspace(workspace); } }🔧 Option 2: Rename the test to match its current behavior
[Test] - public void Execute_Should_Skip_Validation_When_SkipValidation_Is_True() + public void Execute_Should_Fail_On_Unparseable_OpenAPI_Even_When_SkipValidation_Is_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/MSBuild/RefitterGenerateTaskTests.cs` around lines 232 - 262, The test method Execute_Should_Skip_Validation_When_SkipValidation_Is_True has inconsistent logic: it writes unparseable JSON and expects Execute() to return false, but this doesn't actually test validation skipping since JSON parsing fails before validation runs. To fix this, replace the unparseable JSON with valid JSON that would normally fail OpenAPI schema validation (such as JSON missing required OpenAPI fields like "openapi" or "info"), keep SkipValidation set to true, and change the assertion so result.Should().BeTrue() instead of BeFalse(), since the validation should be skipped and the valid JSON should be successfully processed.
🧹 Nitpick comments (5)
docs/prd/decouple-msbuild-from-cli.md (2)
245-245: 💤 Low valueAdd language specifier to fenced code block.
The fenced code block for the target architecture diagram should include a language specifier (e.g.,
```text) to comply with markdown linting rules.🤖 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 `@docs/prd/decouple-msbuild-from-cli.md` at line 245, The fenced code block in the target architecture diagram section is missing a language specifier, which violates markdown linting rules. Add a language specifier such as `text` immediately after the opening triple backticks (change ``` to ```text) for the fenced code block at line 245 to comply with markdown linting requirements.
221-221: 💤 Low valueAdd language specifier to fenced code block.
The fenced code block for the architecture diagram should include a language specifier (e.g.,
```text) to comply with markdown linting rules.🤖 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 `@docs/prd/decouple-msbuild-from-cli.md` at line 221, Add a language specifier to the opening fence of the architecture diagram code block to comply with markdown linting rules. Change the opening fence from ` ``` ` to ` ```text ` (or another appropriate language identifier based on the diagram content) so that the fenced code block follows markdown best practices for syntax highlighting and linting compliance.src/Refitter.Core/OutputPlanner.cs (2)
109-120: 💤 Low valueConsider using the
CombineWithSettingsRoothelper inGetContractsOutputPath.The
GetContractsOutputPathmethod manually combines paths usingPath.GetFullPath(Path.Combine(...))at line 118-119, while theGetMultiFileOutputPathmethod uses theCombineWithSettingsRoothelper (lines 97, 99). This inconsistency could lead to subtle path-resolution differences. Consider refactoring to use the same helper for consistency.🤖 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/OutputPlanner.cs` around lines 109 - 120, The GetContractsOutputPath method is manually constructing the path using Path.GetFullPath(Path.Combine(...)), while the GetMultiFileOutputPath method uses the CombineWithSettingsRoot helper for the same purpose, creating inconsistency in path resolution. Refactor the GetContractsOutputPath method to use the CombineWithSettingsRoot helper instead of the manual path combination, passing the root variable and refitGeneratorSettings.ContractsOutputFolder as arguments, then combine the result with the outputFile.Filename as before.
22-131: ⚡ Quick winReplace
varwith explicit types throughoutOutputPlanner.The coding guidelines discourage use of the
varkeyword (csharp_style_var_* = false:silentper.editorconfig). This file usesvarextensively at lines 22, 25, 48, 54, 61, 75, 81, 88, 92, 114, 118, and 131. Consider replacing with explicit types for consistency with the project's coding standards. As per coding guidelines, "Discourage use ofvarkeyword."🤖 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/OutputPlanner.cs` around lines 22 - 131, Replace all instances of the `var` keyword with explicit types throughout the OutputPlanner class to comply with the project's coding guidelines that discourage use of `var`. Specifically, in the PlanFiles method, replace `var planned` with `List<PlannedFile>` and `var outputFile` with `GeneratedCode`; in GetSingleFileOutputPath, replace `var root` with `string`, `var cliOverridesOutput` with `bool`, and `var filename` with `string`; in GetMultiFileOutputPath, replace `var root` with `string` and `var outputFolder` with `string?`; in GetContractsOutputPath, replace `var root` with `string` and `var contractsFolder` with `string`; and in CombineWithSettingsRoot, replace `var combinedPath` with `string`. This ensures consistency with the project's editorconfig settings which set `csharp_style_var_* = false:silent`.Source: Coding guidelines
src/Refitter.MSBuild/RefitterGenerateTask.cs (1)
67-134: ⚡ Quick winConsider extracting helpers to reduce cognitive complexity.
SonarCloud flags this method's cognitive complexity at 22 (threshold: 15). The method handles settings loading, generation, file I/O, directory creation, and validation. Extracting helpers would improve readability and testability.
The directory creation logic is also duplicated (lines 90-92 and 106-108):
♻️ Suggested helper extraction
private static void EnsureDirectoryExists(string filePath) { var dir = Path.GetDirectoryName(filePath); if (!string.IsNullOrWhiteSpace(dir) && !Directory.Exists(dir)) Directory.CreateDirectory(dir); } private static void WriteGeneratedFile(string outputPath, string content, List<string> generatedFiles) { EnsureDirectoryExists(outputPath); File.WriteAllText(outputPath, content); generatedFiles.Add(Path.GetFullPath(outputPath)); }🤖 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 67 - 134, The ProcessRefitterFile method has a cognitive complexity of 22 (exceeding the threshold of 15) due to handling multiple responsibilities including settings loading, generation, file I/O, and validation. Extract helper methods to reduce complexity and eliminate duplication. Create an EnsureDirectoryExists helper method to encapsulate the duplicated directory creation logic that appears in the multi-file generation branch and single-file generation branch. Create a WriteGeneratedFile helper method that calls EnsureDirectoryExists and handles writing the content to disk and tracking the generated file path. Use these helpers to replace the duplicated directory creation and file writing logic throughout the ProcessRefitterFile method, improving readability and testability.Source: Linters/SAST tools
🤖 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/OutputPlanner.cs`:
- Around line 5-136: Add XML documentation comments to all public methods in the
OutputPlanner class to comply with coding guidelines. For each public method
(PlanSingleFile, PlanMultipleFiles, GetSingleFileOutputPath,
GetMultiFileOutputPath, ShouldRerouteToContractsFolder, and
GetContractsOutputPath), add a triple-slash XML documentation comment above the
method signature that describes the method's purpose, parameters, and return
value. Include appropriate summary, param, and returns tags to fully document
each method's functionality.
In `@src/Refitter.MSBuild/RefitterGenerateTask.cs`:
- Around line 79-131: The validation of the OpenAPI spec runs after files are
written to disk, causing orphaned files to remain if validation fails. Move the
validation logic that checks SkipValidation and calls
OpenApiValidator.Validate() to execute before the file generation blocks (the
if/else blocks that call GenerateMultipleFiles() or Generate()). This way,
invalid OpenAPI specs will throw exceptions before any files are written to
disk, preventing orphaned output. Reorganize the code so the validation block
executes first, then only proceed with generating and writing files if
validation passes.
In `@src/Refitter/GenerateCommand.cs`:
- Around line 92-106: Remove the two private GetOutputPath helper methods from
the GenerateCommand class. The first static GetOutputPath method that takes
Settings and RefitGeneratorSettings parameters, and the second instance
GetOutputPath method that takes Settings, RefitGeneratorSettings, and
GeneratedCode parameters are both unused throughout the codebase and serve only
as unnecessary wrappers around OutputPlanner calls. Since these methods are
private, they cannot be called from outside the file, so they can be safely
deleted without affecting any other code.
---
Outside diff comments:
In `@src/Refitter.MSBuild/Refitter.MSBuild.csproj`:
- Around line 13-27: The PropertyGroup in the Refitter.MSBuild.csproj file is
missing the TreatWarningsAsErrors setting that is required per coding guidelines
and already present in Refitter.Core.csproj. Add a new line containing
<TreatWarningsAsErrors>true</TreatWarningsAsErrors> to the PropertyGroup
section, placing it alongside the existing SuppressDependenciesWhenPacking
setting to ensure the project treats all build warnings as errors.
In `@src/Refitter.Tests/MSBuild/RefitterGenerateTaskTests.cs`:
- Around line 232-262: The test method
Execute_Should_Skip_Validation_When_SkipValidation_Is_True has inconsistent
logic: it writes unparseable JSON and expects Execute() to return false, but
this doesn't actually test validation skipping since JSON parsing fails before
validation runs. To fix this, replace the unparseable JSON with valid JSON that
would normally fail OpenAPI schema validation (such as JSON missing required
OpenAPI fields like "openapi" or "info"), keep SkipValidation set to true, and
change the assertion so result.Should().BeTrue() instead of BeFalse(), since the
validation should be skipped and the valid JSON should be successfully
processed.
---
Nitpick comments:
In `@docs/prd/decouple-msbuild-from-cli.md`:
- Line 245: The fenced code block in the target architecture diagram section is
missing a language specifier, which violates markdown linting rules. Add a
language specifier such as `text` immediately after the opening triple backticks
(change ``` to ```text) for the fenced code block at line 245 to comply with
markdown linting requirements.
- Line 221: Add a language specifier to the opening fence of the architecture
diagram code block to comply with markdown linting rules. Change the opening
fence from ` ``` ` to ` ```text ` (or another appropriate language identifier
based on the diagram content) so that the fenced code block follows markdown
best practices for syntax highlighting and linting compliance.
In `@src/Refitter.Core/OutputPlanner.cs`:
- Around line 109-120: The GetContractsOutputPath method is manually
constructing the path using Path.GetFullPath(Path.Combine(...)), while the
GetMultiFileOutputPath method uses the CombineWithSettingsRoot helper for the
same purpose, creating inconsistency in path resolution. Refactor the
GetContractsOutputPath method to use the CombineWithSettingsRoot helper instead
of the manual path combination, passing the root variable and
refitGeneratorSettings.ContractsOutputFolder as arguments, then combine the
result with the outputFile.Filename as before.
- Around line 22-131: Replace all instances of the `var` keyword with explicit
types throughout the OutputPlanner class to comply with the project's coding
guidelines that discourage use of `var`. Specifically, in the PlanFiles method,
replace `var planned` with `List<PlannedFile>` and `var outputFile` with
`GeneratedCode`; in GetSingleFileOutputPath, replace `var root` with `string`,
`var cliOverridesOutput` with `bool`, and `var filename` with `string`; in
GetMultiFileOutputPath, replace `var root` with `string` and `var outputFolder`
with `string?`; in GetContractsOutputPath, replace `var root` with `string` and
`var contractsFolder` with `string`; and in CombineWithSettingsRoot, replace
`var combinedPath` with `string`. This ensures consistency with the project's
editorconfig settings which set `csharp_style_var_* = false:silent`.
In `@src/Refitter.MSBuild/RefitterGenerateTask.cs`:
- Around line 67-134: The ProcessRefitterFile method has a cognitive complexity
of 22 (exceeding the threshold of 15) due to handling multiple responsibilities
including settings loading, generation, file I/O, and validation. Extract helper
methods to reduce complexity and eliminate duplication. Create an
EnsureDirectoryExists helper method to encapsulate the duplicated directory
creation logic that appears in the multi-file generation branch and single-file
generation branch. Create a WriteGeneratedFile helper method that calls
EnsureDirectoryExists and handles writing the content to disk and tracking the
generated file path. Use these helpers to replace the duplicated directory
creation and file writing logic throughout the ProcessRefitterFile method,
improving readability and testability.
🪄 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: 687ee52c-daa7-408f-9b93-5386ae1661bb
📒 Files selected for processing (36)
.github/workflows/msbuild.ymlAGENTS.mddocs/prd/decouple-msbuild-from-cli.mdsrc/Refitter.Core/OutputPlanner.cssrc/Refitter.Core/RefitterSettingsLoader.cssrc/Refitter.Core/Validation/OpenApiStats.cssrc/Refitter.Core/Validation/OpenApiValidationException.cssrc/Refitter.Core/Validation/OpenApiValidationResult.cssrc/Refitter.Core/Validation/OpenApiValidator.cssrc/Refitter.MSBuild/DefaultProcessRunner.cssrc/Refitter.MSBuild/DefaultRuntimeResolver.cssrc/Refitter.MSBuild/IProcessRunner.cssrc/Refitter.MSBuild/IRuntimeResolver.cssrc/Refitter.MSBuild/ProcessExecutionResult.cssrc/Refitter.MSBuild/Refitter.MSBuild.csprojsrc/Refitter.MSBuild/RefitterGenerateTask.cssrc/Refitter.Tests/GenerateCommandTests.cssrc/Refitter.Tests/GenerationOrchestratorTests.cssrc/Refitter.Tests/MSBuild/RefitterGenerateTaskTests.cssrc/Refitter.Tests/OpenApi/OpenApiStatsTests.cssrc/Refitter.Tests/OpenApi/OpenApiValidationExceptionTests.cssrc/Refitter.Tests/OpenApi/OpenApiValidationResultTests.cssrc/Refitter.Tests/OpenApi/OpenApiValidatorTests.cssrc/Refitter.Tests/OutputPlannerTests.cssrc/Refitter.Tests/RichGenerationReporterTests.cssrc/Refitter.Tests/Scenarios/SettingsFileOutputPathTests.cssrc/Refitter.Tests/SimpleGenerationReporterTests.cssrc/Refitter/GenerateCommand.cssrc/Refitter/GenerationOrchestrator.cssrc/Refitter/IGenerationReporter.cssrc/Refitter/OutputPlanner.cssrc/Refitter/RichGenerationReporter.cssrc/Refitter/SettingsValidator.cssrc/Refitter/SimpleGenerationReporter.cstest/MSBuild/build.ps1test/MSBuild/build.sh
💤 Files with no reviewable changes (9)
- src/Refitter.MSBuild/DefaultRuntimeResolver.cs
- src/Refitter.MSBuild/IProcessRunner.cs
- src/Refitter.MSBuild/ProcessExecutionResult.cs
- src/Refitter.MSBuild/IRuntimeResolver.cs
- src/Refitter/OutputPlanner.cs
- test/MSBuild/build.ps1
- test/MSBuild/build.sh
- src/Refitter.MSBuild/DefaultProcessRunner.cs
- .github/workflows/msbuild.yml
Comment on lines
+5
to
+136
| public static class OutputPlanner | ||
| { | ||
| public const string DefaultOutputPath = "Output.cs"; | ||
|
|
||
| public static PlannedFile PlanSingleFile( | ||
| string? settingsFilePath, | ||
| string? cliOutputPath, | ||
| RefitGeneratorSettings refitGeneratorSettings, | ||
| string code) => | ||
| new(GetSingleFileOutputPath(settingsFilePath, cliOutputPath, refitGeneratorSettings), code); | ||
|
|
||
| public static IReadOnlyList<PlannedFile> PlanMultipleFiles( | ||
| string? settingsFilePath, | ||
| string? cliOutputPath, | ||
| RefitGeneratorSettings refitGeneratorSettings, | ||
| GeneratorOutput generatorOutput) | ||
| { | ||
| var planned = new List<PlannedFile>(generatorOutput.Files.Count); | ||
| foreach (var outputFile in generatorOutput.Files) | ||
| { | ||
| var path = ShouldRerouteToContractsFolder(refitGeneratorSettings, outputFile) | ||
| ? GetContractsOutputPath(settingsFilePath, refitGeneratorSettings, outputFile) | ||
| : GetMultiFileOutputPath(settingsFilePath, cliOutputPath, refitGeneratorSettings, outputFile); | ||
|
|
||
| planned.Add(new PlannedFile(path, outputFile.Content)); | ||
| } | ||
|
|
||
| return planned; | ||
| } | ||
|
|
||
| public static string GetSingleFileOutputPath( | ||
| string? settingsFilePath, | ||
| string? cliOutputPath, | ||
| RefitGeneratorSettings refitGeneratorSettings) | ||
| { | ||
| if (IsDirectCliGeneration(settingsFilePath)) | ||
| { | ||
| if (HasExplicitCliOutputOverride(cliOutputPath)) | ||
| return cliOutputPath!; | ||
|
|
||
| return DefaultOutputPath; | ||
| } | ||
|
|
||
| var root = string.IsNullOrWhiteSpace(settingsFilePath) | ||
| ? string.Empty | ||
| : Path.GetDirectoryName(settingsFilePath) ?? string.Empty; | ||
|
|
||
| var cliOverridesOutput = HasExplicitCliOutputOverride(cliOutputPath); | ||
|
|
||
| string outputPath; | ||
| if (cliOverridesOutput) | ||
| { | ||
| outputPath = cliOutputPath!; | ||
| } | ||
| else | ||
| { | ||
| var filename = refitGeneratorSettings.OutputFilename ?? "Output.cs"; | ||
| outputPath = !string.IsNullOrWhiteSpace(refitGeneratorSettings.OutputFolder) | ||
| ? Path.Combine(refitGeneratorSettings.OutputFolder, filename) | ||
| : filename; | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(root) && !Path.IsPathRooted(outputPath)) | ||
| outputPath = Path.Combine(root, outputPath); | ||
|
|
||
| return outputPath; | ||
| } | ||
|
|
||
| public static string GetMultiFileOutputPath( | ||
| string? settingsFilePath, | ||
| string? cliOutputPath, | ||
| RefitGeneratorSettings refitGeneratorSettings, | ||
| GeneratedCode outputFile) | ||
| { | ||
| if (IsDirectCliGeneration(settingsFilePath)) | ||
| { | ||
| var outputDirectory = HasExplicitCliOutputOverride(cliOutputPath) | ||
| ? cliOutputPath! | ||
| : "."; | ||
|
|
||
| return Path.Combine(outputDirectory, outputFile.Filename); | ||
| } | ||
|
|
||
| var root = string.IsNullOrWhiteSpace(settingsFilePath) | ||
| ? string.Empty | ||
| : Path.GetDirectoryName(settingsFilePath) ?? string.Empty; | ||
|
|
||
| var outputFolder = HasExplicitCliOutputOverride(cliOutputPath) | ||
| ? cliOutputPath | ||
| : refitGeneratorSettings.OutputFolder; | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(outputFolder)) | ||
| return CombineWithSettingsRoot(root, outputFolder!, outputFile.Filename); | ||
|
|
||
| return CombineWithSettingsRoot(root, outputFile.Filename); | ||
| } | ||
|
|
||
| public static bool ShouldRerouteToContractsFolder( | ||
| RefitGeneratorSettings refitGeneratorSettings, | ||
| GeneratedCode outputFile) => | ||
| !string.IsNullOrWhiteSpace(refitGeneratorSettings.ContractsOutputFolder) | ||
| && refitGeneratorSettings.ContractsOutputFolder != RefitGeneratorSettings.DefaultOutputFolder | ||
| && outputFile.Filename == $"{TypenameConstants.Contracts}.cs"; | ||
|
|
||
| public static string GetContractsOutputPath( | ||
| string? settingsFilePath, | ||
| RefitGeneratorSettings refitGeneratorSettings, | ||
| GeneratedCode outputFile) | ||
| { | ||
| var root = string.IsNullOrWhiteSpace(settingsFilePath) | ||
| ? string.Empty | ||
| : Path.GetDirectoryName(settingsFilePath) ?? string.Empty; | ||
|
|
||
| var contractsFolder = Path.GetFullPath(Path.Combine(root, refitGeneratorSettings.ContractsOutputFolder!)); | ||
| return Path.Combine(contractsFolder, outputFile.Filename); | ||
| } | ||
|
|
||
| private static bool IsDirectCliGeneration(string? settingsFilePath) => | ||
| string.IsNullOrWhiteSpace(settingsFilePath); | ||
|
|
||
| private static bool HasExplicitCliOutputOverride(string? cliOutputPath) => | ||
| !string.IsNullOrWhiteSpace(cliOutputPath) && | ||
| cliOutputPath != DefaultOutputPath; | ||
|
|
||
| private static string CombineWithSettingsRoot(string root, params string[] segments) | ||
| { | ||
| var combinedPath = Path.Combine(segments); | ||
| return !string.IsNullOrWhiteSpace(root) && !Path.IsPathRooted(combinedPath) | ||
| ? Path.Combine(root, combinedPath) | ||
| : combinedPath; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add XML documentation for all public methods in OutputPlanner.
The coding guidelines require XML documentation for public APIs. All public methods in this new OutputPlanner class lack XML documentation 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/OutputPlanner.cs` around lines 5 - 136, Add XML
documentation comments to all public methods in the OutputPlanner class to
comply with coding guidelines. For each public method (PlanSingleFile,
PlanMultipleFiles, GetSingleFileOutputPath, GetMultiFileOutputPath,
ShouldRerouteToContractsFolder, and GetContractsOutputPath), add a triple-slash
XML documentation comment above the method signature that describes the method's
purpose, parameters, and return value. Include appropriate summary, param, and
returns tags to fully document each method's functionality.
Source: Coding guidelines
Comment on lines
+79
to
131
| if (settings.GenerateMultipleFiles) | ||
| { | ||
| FileName = "dotnet", | ||
| Arguments = args, | ||
| WorkingDirectory = Path.GetDirectoryName(file)!, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| RedirectStandardInput = true, | ||
| UseShellExecute = false, | ||
| CreateNoWindow = true, | ||
| }; | ||
|
|
||
| var processResult = ProcessRunner.Run( | ||
| startInfo, | ||
| data => HandleProcessStandardOutput(data, outputLines, outputLines, TryLogCommandLine), | ||
| data => HandleProcessErrorOutput(data, TryLogError)); | ||
|
|
||
| if (processResult.TimedOut) | ||
| { | ||
| failed = true; | ||
| var timeoutDescription = FormatTimeout(ProcessTimeoutMilliseconds); | ||
| TryLogError( | ||
| processResult.TerminationException is null | ||
| ? $"Refitter process timed out after {timeoutDescription} and was terminated" | ||
| : $"Refitter process timed out after {timeoutDescription}. Failed to terminate timed-out process: {processResult.TerminationException.Message}"); | ||
|
|
||
| return new(); | ||
| var output = generator.GenerateMultipleFiles(); | ||
| foreach (var outputFile in output.Files) | ||
| { | ||
| var outputPath = OutputPlanner.GetMultiFileOutputPath( | ||
| filePath, | ||
| cliOutputPath: null, | ||
| settings, | ||
| outputFile); | ||
|
|
||
| var dir = Path.GetDirectoryName(outputPath); | ||
| if (!string.IsNullOrWhiteSpace(dir) && !Directory.Exists(dir)) | ||
| Directory.CreateDirectory(dir); | ||
|
|
||
| File.WriteAllText(outputPath, outputFile.Content); | ||
| generatedFiles.Add(Path.GetFullPath(outputPath)); | ||
| } | ||
| } | ||
|
|
||
| if (processResult.ExitCode != 0) | ||
| else | ||
| { | ||
| failed = true; | ||
| TryLogError($"Refitter process exited with code {processResult.ExitCode}"); | ||
| return new List<string>(); | ||
| } | ||
| var code = generator.Generate().Replace("\r\n", "\n"); | ||
| var outputPath = OutputPlanner.GetSingleFileOutputPath( | ||
| filePath, | ||
| cliOutputPath: null, | ||
| settings); | ||
|
|
||
| return ResolveGeneratedFiles(outputLines, file, out failed, TryLogError); | ||
| } | ||
| var dir = Path.GetDirectoryName(outputPath); | ||
| if (!string.IsNullOrWhiteSpace(dir) && !Directory.Exists(dir)) | ||
| Directory.CreateDirectory(dir); | ||
|
|
||
| internal static string? ResolveRefitterDll( | ||
| string? packageFolder, | ||
| IReadOnlyList<string>? installedRuntimes, | ||
| Action<string> logCommandLine, | ||
| Func<string, bool> fileExists) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(packageFolder)) | ||
| { | ||
| return null; | ||
| File.WriteAllText(outputPath, code); | ||
| generatedFiles.Add(Path.GetFullPath(outputPath)); | ||
| } | ||
|
|
||
| var bundledRuntimes = PreferredRuntimeOrder | ||
| .Select(candidate => new | ||
| { | ||
| candidate.TargetFramework, | ||
| candidate.RuntimePrefix, | ||
| Path = Path.GetFullPath(Path.Combine(packageFolder, "..", candidate.TargetFramework, "refitter.dll")), | ||
| }) | ||
| .ToArray(); | ||
|
|
||
| if (installedRuntimes is not null) | ||
| if (!SkipValidation) | ||
| { | ||
| var detectedRuntimes = installedRuntimes | ||
| .Where(installed => !string.IsNullOrWhiteSpace(installed)) | ||
| .ToArray(); | ||
| var openApiPaths = settings.OpenApiPaths is { Length: > 0 } | ||
| ? settings.OpenApiPaths | ||
| : settings.OpenApiPath is not null | ||
| ? [settings.OpenApiPath] | ||
| : []; | ||
|
|
||
| foreach (var runtime in bundledRuntimes.Where(runtime => fileExists(runtime.Path))) | ||
| foreach (var specPath in openApiPaths) | ||
| { | ||
| if (detectedRuntimes.Any(installed => | ||
| installed.StartsWith(runtime.RuntimePrefix, StringComparison.Ordinal))) | ||
| if (!string.IsNullOrWhiteSpace(specPath)) | ||
| { | ||
| logCommandLine($"Detected {GetDisplayFramework(runtime.TargetFramework)} runtime. Using {GetDisplayFramework(runtime.TargetFramework)} version of Refitter."); | ||
| return runtime.Path; | ||
| var validationResult = OpenApiValidator.Validate(specPath) | ||
| .GetAwaiter().GetResult(); | ||
| validationResult.ThrowIfInvalid(); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Validation runs after generation, leaving orphaned files on failure.
The current flow writes generated files to disk (lines 79-112) before validating the OpenAPI spec (lines 114-131). If validation fails and throws, the exception bubbles up to Execute() which sets hasErrors = true and returns false, but the generated files remain on disk.
Consider validating first so invalid specs don't produce partial/orphaned output:
♻️ Suggested refactor: validate before generating
private List<string> ProcessRefitterFile(string filePath)
{
var baseDirectory = Path.GetDirectoryName(Path.GetFullPath(filePath))!;
var json = File.ReadAllText(filePath);
var settings = RefitterSettingsLoader.Load(json, baseDirectory);
RefitterSettingsLoader.ApplyDefaults(filePath, settings);
+ if (!SkipValidation)
+ {
+ ValidateOpenApiSpecs(settings);
+ }
+
var generator = RefitGenerator.CreateAsync(settings)
.GetAwaiter().GetResult();
var generatedFiles = new List<string>();
if (settings.GenerateMultipleFiles)
{
// ... generation logic ...
}
else
{
// ... generation logic ...
}
- if (!SkipValidation)
- {
- // ... validation logic ...
- }
-
return generatedFiles;
}
+
+private void ValidateOpenApiSpecs(RefitterSettings settings)
+{
+ var openApiPaths = settings.OpenApiPaths is { Length: > 0 }
+ ? settings.OpenApiPaths
+ : settings.OpenApiPath is not null
+ ? new[] { settings.OpenApiPath }
+ : Array.Empty<string>();
+
+ foreach (var specPath in openApiPaths.Where(p => !string.IsNullOrWhiteSpace(p)))
+ {
+ var validationResult = OpenApiValidator.Validate(specPath)
+ .GetAwaiter().GetResult();
+ validationResult.ThrowIfInvalid();
+ }
+}🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 122-122: Loops should be simplified using the "Where" LINQ method
[warning] 118-120: Extract this nested ternary operation into an independent statement.
🤖 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 79 - 131, The
validation of the OpenAPI spec runs after files are written to disk, causing
orphaned files to remain if validation fails. Move the validation logic that
checks SkipValidation and calls OpenApiValidator.Validate() to execute before
the file generation blocks (the if/else blocks that call GenerateMultipleFiles()
or Generate()). This way, invalid OpenAPI specs will throw exceptions before any
files are written to disk, preventing orphaned output. Reorganize the code so
the validation block executes first, then only proceed with generating and
writing files if validation passes.
Comment on lines
92
to
+106
| private static string GetOutputPath(Settings settings, RefitGeneratorSettings refitGeneratorSettings) => | ||
| OutputPlanner.GetSingleFileOutputPath(settings, refitGeneratorSettings); | ||
| OutputPlanner.GetSingleFileOutputPath( | ||
| settings.SettingsFilePath, | ||
| settings.OutputPath, | ||
| refitGeneratorSettings); | ||
|
|
||
| private string GetOutputPath( | ||
| Settings settings, | ||
| RefitGeneratorSettings refitGeneratorSettings, | ||
| GeneratedCode outputFile) => | ||
| OutputPlanner.GetMultiFileOutputPath(settings, refitGeneratorSettings, outputFile); | ||
|
|
||
| internal static void ApplySettingsFileDefaults(string settingsFilePath, RefitGeneratorSettings refitGeneratorSettings) | ||
| { | ||
| if (!string.IsNullOrWhiteSpace(refitGeneratorSettings.ContractsOutputFolder)) | ||
| refitGeneratorSettings.GenerateMultipleFiles = true; | ||
|
|
||
| if (string.IsNullOrWhiteSpace(refitGeneratorSettings.OutputFolder)) | ||
| refitGeneratorSettings.OutputFolder = RefitGeneratorSettings.DefaultOutputFolder; | ||
|
|
||
| if (string.IsNullOrWhiteSpace(refitGeneratorSettings.OutputFilename)) | ||
| { | ||
| var refitterFileName = Path.GetFileNameWithoutExtension(settingsFilePath); | ||
| if (string.IsNullOrEmpty(refitterFileName)) | ||
| refitterFileName = "Output"; | ||
| refitGeneratorSettings.OutputFilename = $"{refitterFileName}.cs"; | ||
| } | ||
| } | ||
| OutputPlanner.GetMultiFileOutputPath( | ||
| settings.SettingsFilePath, | ||
| settings.OutputPath, | ||
| refitGeneratorSettings, | ||
| outputFile); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any remaining calls to these private GetOutputPath helpers
rg -n 'GetOutputPath\s*\(' --type csRepository: christianhelle/refitter
Length of output: 1012
🏁 Script executed:
cd src/Refitter && wc -l GenerateCommand.csRepository: christianhelle/refitter
Length of output: 90
🏁 Script executed:
# Read the GenerateCommand.cs file to see the context and any calls to GetOutputPath
cat -n src/Refitter/GenerateCommand.cs | head -150 | tail -100Repository: christianhelle/refitter
Length of output: 4771
🏁 Script executed:
# Check if GetOutputPath is called in GenerateCommand.cs with different search patterns
rg -n 'GetOutputPath' src/Refitter/GenerateCommand.csRepository: christianhelle/refitter
Length of output: 216
Remove these unused private GetOutputPath helper methods.
The two private GetOutputPath methods (lines 92–106) are not called anywhere in this file or the rest of the codebase. Since they're private, they cannot be invoked from other files. They appear to be obsolete wrappers around OutputPlanner calls.
🤖 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/GenerateCommand.cs` around lines 92 - 106, Remove the two
private GetOutputPath helper methods from the GenerateCommand class. The first
static GetOutputPath method that takes Settings and RefitGeneratorSettings
parameters, and the second instance GetOutputPath method that takes Settings,
RefitGeneratorSettings, and GeneratedCode parameters are both unused throughout
the codebase and serve only as unnecessary wrappers around OutputPlanner calls.
Since these methods are private, they cannot be called from outside the file, so
they can be safely deleted without affecting any other code.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1154 +/- ##
==========================================
+ Coverage 94.57% 94.74% +0.16%
==========================================
Files 65 69 +4
Lines 3095 3156 +61
==========================================
+ Hits 2927 2990 +63
- Misses 58 62 +4
+ Partials 110 104 -6
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:
|
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
christianhelle
added
enhancement
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Refitter.MSBuild/Refitter.MSBuild.csproj (1)
4-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
TreatWarningsAsErrorsproperty.Per coding guidelines, production projects under
src/Refitter.MSBuild/should enableTreatWarningsAsErrors. This property is not present in thePropertyGroup.🛠️ Proposed fix
<PropertyGroup> <TargetFramework>netstandard2.0</TargetFramework> + <TreatWarningsAsErrors>true</TreatWarningsAsErrors> <Product>Refitter MSBuild Tasks</Product>🤖 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/Refitter.MSBuild.csproj` around lines 4 - 16, The PropertyGroup in the Refitter.MSBuild.csproj file is missing the TreatWarningsAsErrors property required by coding guidelines. Add the TreatWarningsAsErrors property set to true within the PropertyGroup section that contains TargetFramework, Product, PackageId, AssemblyName, RootNamespace, Title, Description, GenerateDependencyFile, BuildOutputTargetFolder, CopyLocalLockFileAssemblies, and SuppressDependenciesWhenPacking properties.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.
Outside diff comments:
In `@src/Refitter.MSBuild/Refitter.MSBuild.csproj`:
- Around line 4-16: The PropertyGroup in the Refitter.MSBuild.csproj file is
missing the TreatWarningsAsErrors property required by coding guidelines. Add
the TreatWarningsAsErrors property set to true within the PropertyGroup section
that contains TargetFramework, Product, PackageId, AssemblyName, RootNamespace,
Title, Description, GenerateDependencyFile, BuildOutputTargetFolder,
CopyLocalLockFileAssemblies, and SuppressDependenciesWhenPacking properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afee6881-27d3-4f4f-9c4e-71c92df15983
📒 Files selected for processing (4)
.gitignoresrc/Refitter.MSBuild/Refitter.MSBuild.csprojsrc/Refitter.Tests/OutputPlannerTests.cssrc/Refitter.Tests/SettingsFile/RefitterSettingsLoaderTests.cs
✅ Files skipped from review due to trivial changes (1)
- .gitignore
This was referenced Jun 18, 2026
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 by CodeRabbit
Summary by CodeRabbit