Refactor GenerateCommand into focused modules and stabilize package-reference builds#1095
Conversation
…nner, and FileWriter
📝 WalkthroughWalkthroughRefactors GenerateCommand output and path logic into reusable components: RefitterSettingsLoader for settings/spec-path resolution, OutputPlanner for planned output paths, IGenerationReporter with rich/simple implementations for CLI output, FileWriter for writes, plus tests and a docs update. ChangesOutput and reporting abstraction refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the CLI generation command by moving path planning, file writing, presentation/reporting, and shared settings-path resolution into focused modules, while also updating contributor instructions for agent commits.
Changes:
- Extracts CLI output/reporting into
IGenerationReporterwith simple and rich implementations. - Moves output path decisions into
OutputPlannerand generated file writes intoFileWriter. - Adds
RefitterSettingsLoaderin Core and delegates source-generator relative spec path resolution to it.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Refitter/GenerateCommand.cs |
Replaces inline output, validation, writing, and path-planning logic with the new helper modules. |
src/Refitter/IGenerationReporter.cs |
Defines the reporting seam used by the command orchestration. |
src/Refitter/SimpleGenerationReporter.cs |
Implements deterministic plain-text CLI reporting. |
src/Refitter/RichGenerationReporter.cs |
Implements Spectre.Console rich CLI reporting. |
src/Refitter/OutputPlanner.cs |
Centralizes generated output path planning. |
src/Refitter/FileWriter.cs |
Adds a small filesystem sink for generated files. |
src/Refitter/OpenApiStatisticsFormatter.cs |
Shares OpenAPI statistics parsing and display metadata between reporters. |
src/Refitter.Core/RefitterSettingsLoader.cs |
Adds shared .refitter JSON loading and relative spec path resolution. |
src/Refitter.SourceGenerator/RefitterSourceGenerator.cs |
Delegates relative OpenAPI spec path resolution to Core. |
src/Refitter.Tests/OutputPlannerTests.cs |
Adds coverage for output path planning behavior. |
src/Refitter.Tests/SettingsFile/RefitterSettingsLoaderTests.cs |
Adds coverage for settings loading and path resolution. |
.github/copilot-instructions.md |
Adds agent commit policy instructions. |
| var settings = Serializer.Deserialize<RefitGeneratorSettings>(json); | ||
| ResolveRelativeSpecPaths(settings, baseDirectory); | ||
| return settings; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1095 +/- ##
==========================================
- Coverage 95.90% 95.31% -0.59%
==========================================
Files 27 28 +1
Lines 2590 3012 +422
==========================================
+ Hits 2484 2871 +387
- Misses 36 45 +9
- Partials 70 96 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/RefitterSettingsLoader.cs`:
- Around line 25-29: The Load method must guard against a null result from
Serializer.Deserialize<RefitGeneratorSettings>(json) and throw a JsonException
(or rethrow a JsonException-like error) when json deserializes to null; update
Load to check if settings is null and throw accordingly before calling
ResolveRelativeSpecPaths. Also harden ResolveRelativeSpecPaths to handle a null
or empty settings.OpenApiPaths collection and to skip null/whitespace elements
(check for settings.OpenApiPaths == null and use string.IsNullOrWhiteSpace(path)
before calling Path.IsPathRooted(path)), so null list or null entries do not
cause ArgumentNullException.
- Around line 47-55: In ResolveRelativeSpecPaths, skip null or whitespace
entries in settings.OpenApiPaths before calling Path.IsPathRooted or
Path.Combine: add a guard like if (string.IsNullOrWhiteSpace(path)) continue; so
you don't call Path.IsPathRooted(null) or normalize empty strings into
baseDirectory; keep the IsUrl(path) check and only update
settings.OpenApiPaths[i] when path is non-empty, non-URL and non-rooted.
In `@src/Refitter/GenerateCommand.cs`:
- Around line 79-80: The header is currently reported unconditionally via
_reporter.ReportHeader(version), which ignores the settings.NoBanner flag; wrap
the ReportHeader call in a guard so it only runs when !settings.NoBanner (e.g.,
if (!settings.NoBanner) _reporter.ReportHeader(version)), and apply the same
guard to the later header/donation panel calls referenced around the 140-143
region so the ASCII art banner is suppressed when settings.NoBanner is true;
locate these calls in GenerateCommand / GenerateCommand.cs and update them to
check settings.NoBanner before invoking _reporter.ReportHeader or printing the
banner.
- Around line 83-87: The code currently constructs a placeholder supportKey and
calls _reporter.ReportSupportKey even when settings.NoLogging is true; change
the logic so that when settings.NoLogging is true you skip calling
SupportInformation.GetSupportKey() and do not call _reporter.ReportSupportKey at
all. Locate the block using settings.NoLogging,
SupportInformation.GetSupportKey(), and _reporter.ReportSupportKey(supportKey)
and wrap the reporter call in a conditional that only executes when logging is
enabled, ensuring no placeholder string is created or emitted under
--no-logging.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc31d557-bbf7-4349-a5c4-ab92306d252f
📒 Files selected for processing (12)
.github/copilot-instructions.mdsrc/Refitter.Core/RefitterSettingsLoader.cssrc/Refitter.SourceGenerator/RefitterSourceGenerator.cssrc/Refitter.Tests/OutputPlannerTests.cssrc/Refitter.Tests/SettingsFile/RefitterSettingsLoaderTests.cssrc/Refitter/FileWriter.cssrc/Refitter/GenerateCommand.cssrc/Refitter/IGenerationReporter.cssrc/Refitter/OpenApiStatisticsFormatter.cssrc/Refitter/OutputPlanner.cssrc/Refitter/RichGenerationReporter.cssrc/Refitter/SimpleGenerationReporter.cs
| public static RefitGeneratorSettings Load(string json, string baseDirectory) | ||
| { | ||
| var settings = Serializer.Deserialize<RefitGeneratorSettings>(json); | ||
| ResolveRelativeSpecPaths(settings, baseDirectory); | ||
| return settings; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In System.Text.Json for modern .NET, what does JsonSerializer.Deserialize("null")return for a reference typeT? Does it throw, or return null?
💡 Result:
In System.Text.Json, calling JsonSerializer.Deserialize("null") for a reference type T will return null [1][2]. It does not throw an exception because "null" is considered valid JSON [1][2]. This behavior occurs because the JSON string "null" is a valid JSON value that represents a null reference in.NET [1][2]. Consequently, the deserializer returns a null reference of type T [2]. This is why the return type of the Deserialize method is marked as nullable (T?) in the.NET API documentation; the method must account for the possibility of returning null when the input JSON is explicitly "null" [1][2].
Citations:
- 1: https://stackoverflow.com/questions/72277063/can-the-result-of-jsonserializer-deserialize-ever-be-null
- 2: https://stackoverflow.com/questions/71162382/why-are-the-return-types-of-nets-system-text-json-jsonserializer-deserialize-m
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant parts of the file to confirm behavior for null settings and OpenApiPaths handling
FILE="src/Refitter.Core/RefitterSettingsLoader.cs"
echo "=== File: $FILE ==="
stat -c "%s bytes" "$FILE" 2>/dev/null || true
echo
sed -n '1,200p' "$FILE" | nl -baRepository: christianhelle/refitter
Length of output: 188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Refitter.Core/RefitterSettingsLoader.cs"
echo "=== ls/exists ==="
ls -l "$FILE" || true
echo
echo "=== methods via rg ==="
rg -n "static\s+RefitGeneratorSettings\s+Load|ResolveRelativeSpecPaths|IsUrl" "$FILE" || true
echo
echo "=== lines 1-120 ==="
sed -n '1,120p' "$FILE" | cat
echo
echo "=== lines 20-80 ==="
sed -n '20,80p' "$FILE" | cat
echo
echo "=== lines 80-200 (if any) ==="
sed -n '80,200p' "$FILE" | catRepository: christianhelle/refitter
Length of output: 6611
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore-vcs "class\s+Serializer|static\s+class\s+Serializer|Serializer\.Deserialize<" src/Refitter.CoreRepository: christianhelle/refitter
Length of output: 372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,200p' src/Refitter.Core/Serializer.cs | cat
echo "=== RefitGeneratorSettings type ==="
rg -n "class\s+RefitGeneratorSettings|record\s+RefitGeneratorSettings|struct\s+RefitGeneratorSettings" -S src/Refitter.Core
# Print the matching file(s)
fd -e cs -a "RefitGeneratorSettings" src/Refitter.Core | head -n 20
# Fallback: search OpenApiPaths property definition
echo "=== OpenApiPaths property ==="
rg -n "OpenApiPaths" src/Refitter.Core -SRepository: christianhelle/refitter
Length of output: 3606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Refitter.Core/Settings/RefitGeneratorSettings.cs"
sed -n '1,220p' "$FILE" | catRepository: christianhelle/refitter
Length of output: 9193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore-vcs "RefitterSettingsLoader\.Load\(" -S srcRepository: christianhelle/refitter
Length of output: 360
🌐 Web query:
In .NET, what happens when calling System.IO.Path.IsPathRooted with a null string argument? Does it throw or return false?
💡 Result:
Calling System.IO.Path.IsPathRooted with a null string argument throws an ArgumentNullException; it does not return false. [1][2] In the current.NET implementation, the string overload uses ArgumentNullException.ThrowIfNull(path) before checking whether the path contains a root. [1] Microsoft Learn’s API docs also state that the method’s string overload throws ArgumentNullException when path is null. [2]
Citations:
- 1: https://source.dot.net/System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/IO/Path.cs.html
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.io.path.ispathrooted?view=netframework-4.8.1
🌐 Web query:
In .NET, what happens when calling System.IO.Path.Combine with a null segment (e.g., Path.Combine("base", (string)null))? Does it throw ArgumentNullException?
💡 Result:
Yes—System.IO.Path.Combine throws ArgumentNullException when any path segment argument it receives is null (including calls like Path.Combine("base", (string)null)). Microsoft’s API docs state that path1 or path2 is null as a parameter error for Path.Combine, which corresponds to ArgumentNullException.[1][2] In other words: - Path.Combine("base", (string)null) => ArgumentNullException (for the null segment argument, e.g., path2). [1][2] Note: This is for the string-based overloads described in the docs (Combine(String, String), and the multi-parameter overloads similarly validate their inputs).
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.io.path.combine?view=net-8.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.io.path.combine?view=net-10.0
🌐 Web query:
In .NET, what does Uri.TryCreate return when the input string is null? Does it throw?
💡 Result:
Uri.TryCreate(string? uriString, UriKind uriKind, out Uri? result) returns false when the input string is null, and it does not throw. Reason: the Microsoft Learn documentation describes TryCreate as returning a bool indicating success and not throwing for invalid URIs, and the reference source implementation checks if (uriString is null) { return null; }, which causes TryCreate to fail and leave result unset/null. [1][2] So for null input, expect: - Return value: false [1] - out parameter: null (uninitialized / null) [1][2] - Exceptions: none [1]
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.uri.trycreate?view=net-8.0
- 2: https://github.com/dotnet/dotnet/blob/17d11de66cf75b962995c81dd1235fae9aa5ece0/src/runtime/src/libraries/System.Private.Uri/src/System/UriExt.cs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Refitter.Tests/SettingsFile/RefitterSettingsLoaderTests.cs"
ls -l "$FILE" || true
sed -n '1,200p' "$FILE" | catRepository: christianhelle/refitter
Length of output: 2992
Guard against null deserialized settings and null elements in OpenApiPaths
Loaddereferencessettingseven when the JSON payload is the valid literal"null":Serializer.Deserialize<RefitGeneratorSettings>("null")returnsnull, soResolveRelativeSpecPaths(settings, ...)throwsNullReferenceExceptioninstead of aJsonException.ResolveRelativeSpecPathsiteratessettings.OpenApiPathswithout handlingnull/blank elements; anullelement will reachPath.IsPathRooted(path)and throwArgumentNullException.
Proposed fix
public static RefitGeneratorSettings Load(string json, string baseDirectory)
{
- var settings = Serializer.Deserialize<RefitGeneratorSettings>(json);
+ var settings = Serializer.Deserialize<RefitGeneratorSettings>(json)
+ ?? throw new System.Text.Json.JsonException("The .refitter settings payload must deserialize to an object.");
ResolveRelativeSpecPaths(settings, baseDirectory);
return settings;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static RefitGeneratorSettings Load(string json, string baseDirectory) | |
| { | |
| var settings = Serializer.Deserialize<RefitGeneratorSettings>(json); | |
| ResolveRelativeSpecPaths(settings, baseDirectory); | |
| return settings; | |
| public static RefitGeneratorSettings Load(string json, string baseDirectory) | |
| { | |
| var settings = Serializer.Deserialize<RefitGeneratorSettings>(json) | |
| ?? throw new System.Text.Json.JsonException("The .refitter settings payload must deserialize to an object."); | |
| ResolveRelativeSpecPaths(settings, baseDirectory); | |
| return settings; | |
| } |
🤖 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/RefitterSettingsLoader.cs` around lines 25 - 29, The Load
method must guard against a null result from
Serializer.Deserialize<RefitGeneratorSettings>(json) and throw a JsonException
(or rethrow a JsonException-like error) when json deserializes to null; update
Load to check if settings is null and throw accordingly before calling
ResolveRelativeSpecPaths. Also harden ResolveRelativeSpecPaths to handle a null
or empty settings.OpenApiPaths collection and to skip null/whitespace elements
(check for settings.OpenApiPaths == null and use string.IsNullOrWhiteSpace(path)
before calling Path.IsPathRooted(path)), so null list or null entries do not
cause ArgumentNullException.
| if (settings.OpenApiPaths is { Length: > 0 }) | ||
| { | ||
| for (var i = 0; i < settings.OpenApiPaths.Length; i++) | ||
| { | ||
| var path = settings.OpenApiPaths[i]; | ||
| if (!IsUrl(path) && !Path.IsPathRooted(path)) | ||
| { | ||
| settings.OpenApiPaths[i] = Path.GetFullPath(Path.Combine(baseDirectory, path)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In .NET, what do Path.IsPathRooted(null), Path.Combine(baseDirectory, ""), and Path.Combine(baseDirectory, null) do?
💡 Result:
In.NET, the behaviors for these Path methods are as follows: Path.IsPathRooted(null) Calling Path.IsPathRooted with a null argument throws an ArgumentNullException [1][2][3]. Path.Combine(baseDirectory, "") If the second argument is an empty string (""), Path.Combine returns the baseDirectory (optionally with a directory separator character appended if it is missing) [4][5][6]. It does not throw an exception for an empty string [6]. Path.Combine(baseDirectory, null) Calling Path.Combine with a null argument for any of its path parameters throws an ArgumentNullException [7]. Unlike the empty string, a null value is not treated as a valid path component [6][7].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.io.path.ispathrooted?view=net-9.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.io.path.ispathrooted?view=netframework-4.8.1
- 3: https://learn.microsoft.com/en-us/previous-versions/windows/embedded/t070a83t(v=vs.102)
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.io.path.combine?view=net-8.0
- 5: https://learn.microsoft.com/en-us/dotnet/api/system.io.path.combine?view=net-10.0
- 6: https://startbigthinksmall.wordpress.com/2008/05/28/pathcombine-does-more-then-just-putting-a-between-to-strings-use-it/
- 7: https://stackoverflow.com/questions/59289740/c-sharp-value-cannot-be-null-using-path-combine
Skip null/whitespace entries in OpenApiPaths (in ResolveRelativeSpecPaths)
Path.IsPathRooted(null) throws ArgumentNullException, and empty/whitespace values get normalized into baseDirectory via Path.Combine(baseDirectory, ""), so malformed entries are rewritten instead of being left for validation.
Proposed fix
if (settings.OpenApiPaths is { Length: > 0 })
{
for (var i = 0; i < settings.OpenApiPaths.Length; i++)
{
var path = settings.OpenApiPaths[i];
- if (!IsUrl(path) && !Path.IsPathRooted(path))
+ if (string.IsNullOrWhiteSpace(path) || IsUrl(path) || Path.IsPathRooted(path))
{
- settings.OpenApiPaths[i] = Path.GetFullPath(Path.Combine(baseDirectory, path));
+ continue;
}
+
+ settings.OpenApiPaths[i] = Path.GetFullPath(Path.Combine(baseDirectory, 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.Core/RefitterSettingsLoader.cs` around lines 47 - 55, In
ResolveRelativeSpecPaths, skip null or whitespace entries in
settings.OpenApiPaths before calling Path.IsPathRooted or Path.Combine: add a
guard like if (string.IsNullOrWhiteSpace(path)) continue; so you don't call
Path.IsPathRooted(null) or normalize empty strings into baseDirectory; keep the
IsUrl(path) check and only update settings.OpenApiPaths[i] when path is
non-empty, non-URL and non-rooted.
| // Header with branding | ||
| if (settings.SimpleOutput) | ||
| { | ||
| Console.WriteLine(); | ||
| Console.WriteLine($"Refitter v{version}"); | ||
| Console.WriteLine("OpenAPI to Refit Interface Generator"); | ||
| Console.WriteLine(); | ||
| } | ||
| else | ||
| { | ||
| AnsiConsole.WriteLine(); | ||
| AnsiConsole.MarkupLine($"[bold cyan]{AsciiArt}[/]"); | ||
| AnsiConsole.MarkupLine($"[bold cyan]╔═══════════════════════════════════════════════════════════════╗[/]"); | ||
| AnsiConsole.MarkupLine($"[bold cyan]║[/] [bold white]🚀 Refitter v{version,-48}[/] [bold cyan]║[/]"); | ||
| AnsiConsole.MarkupLine($"[bold cyan]║[/] [dim] OpenAPI to Refit Interface Generator[/]{new string(' ', 22)} [bold cyan]║[/]"); | ||
| AnsiConsole.MarkupLine($"[bold cyan]╚═══════════════════════════════════════════════════════════════╝[/]"); | ||
| AnsiConsole.WriteLine(); | ||
| } | ||
| _reporter.ReportHeader(version); |
There was a problem hiding this comment.
Honor --no-banner for the startup banner.
Line 80 now reports the header unconditionally, so --no-banner no longer suppresses the startup banner and only affects the donation panel later. The header call should be gated by !settings.NoBanner.
As per coding guidelines src/Refitter/**: ASCII art banner is displayed on CLI startup and can be disabled with --no-banner.
Also applies to: 140-143
🤖 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 79 - 80, The header is
currently reported unconditionally via _reporter.ReportHeader(version), which
ignores the settings.NoBanner flag; wrap the ReportHeader call in a guard so it
only runs when !settings.NoBanner (e.g., if (!settings.NoBanner)
_reporter.ReportHeader(version)), and apply the same guard to the later
header/donation panel calls referenced around the 140-143 region so the ASCII
art banner is suppressed when settings.NoBanner is true; locate these calls in
GenerateCommand / GenerateCommand.cs and update them to check settings.NoBanner
before invoking _reporter.ReportHeader or printing the banner.
| var supportKey = settings.NoLogging | ||
| ? "Unavailable when logging is disabled" | ||
| : SupportInformation.GetSupportKey(); | ||
|
|
||
| if (settings.SimpleOutput) | ||
| { | ||
| Console.WriteLine($"Support key: {supportKey}"); | ||
| Console.WriteLine(); | ||
| } | ||
| else | ||
| { | ||
| AnsiConsole.MarkupLine($"[dim]🔑 Support key: {supportKey}[/]"); | ||
| AnsiConsole.WriteLine(); | ||
| } | ||
| _reporter.ReportSupportKey(supportKey); |
There was a problem hiding this comment.
Skip support-key output entirely under --no-logging.
This now prints a support-key message even when logging is disabled. The CLI contract is to disable support-key display in that mode, so the reporter call should be skipped instead of emitting a placeholder string.
As per coding guidelines src/Refitter/**: CLI supports key display for troubleshooting (disabled when --no-logging is used).
🤖 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 83 - 87, The code currently
constructs a placeholder supportKey and calls _reporter.ReportSupportKey even
when settings.NoLogging is true; change the logic so that when
settings.NoLogging is true you skip calling SupportInformation.GetSupportKey()
and do not call _reporter.ReportSupportKey at all. Locate the block using
settings.NoLogging, SupportInformation.GetSupportKey(), and
_reporter.ReportSupportKey(supportKey) and wrap the reporter call in a
conditional that only executes when logging is enabled, ensuring no placeholder
string is created or emitted under --no-logging.
christianhelle
changed the title
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 4 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
|
@copilot Investigate why the build fails and fix the problem |
Fixed in 9ddff10. The failing Build workflow was caused by |
…SimpleGenerationReporter, and RefitterSettingsLoader
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Refitter.Tests/FileWriterTests.cs (1)
8-9: ⚡ Quick winConsider adding TestRoot cleanup.
The
TestRootdirectory is created implicitly when tests run but never cleaned up. Over time, failed test runs could leave orphaned GUID subdirectories underrefitter-filewriter-tests. Consider adding a one-time cleanup in a test class setup/teardown hook, or usingPath.GetTempPath()directly with unique GUIDs (like the other tests do) instead of a sharedTestRoot.🤖 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/FileWriterTests.cs` around lines 8 - 9, The TestRoot constant in FileWriterTests creates a shared temp directory that is never cleaned up; update the test class (FileWriterTests) to either perform one-time cleanup of TestRoot in a test fixture setup/teardown (e.g., remove leftover GUID subdirectories under the directory referenced by TestRoot) or stop using a shared TestRoot and instead generate per-test unique temp directories using Path.GetTempPath() + Guid.NewGuid() where the tests currently reference TestRoot; ensure the chosen approach deletes created directories after tests complete to avoid orphaned temp folders.src/Refitter.Tests/SimpleGenerationReporterTests.cs (1)
33-37: 💤 Low valueConsider using consistent async test pattern.
For consistency with the synchronous "does not throw" tests, consider wrapping the async call:
♻️ Suggested pattern for consistency
[Test] public async Task ReportSingleFileGenerationProgressAsync_Does_Not_Throw() { - await new SimpleGenerationReporter().ReportSingleFileGenerationProgressAsync(); + var act = async () => await new SimpleGenerationReporter().ReportSingleFileGenerationProgressAsync(); + + await act.Should().NotThrowAsync(); }🤖 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/SimpleGenerationReporterTests.cs` around lines 33 - 37, The test currently awaits ReportSingleFileGenerationProgressAsync directly; change it to use NUnit's async "does not throw" pattern by wrapping the call in Assert.DoesNotThrowAsync so the test method ReportSingleFileGenerationProgressAsync_Does_Not_Throw uses Assert.DoesNotThrowAsync(() => new SimpleGenerationReporter().ReportSingleFileGenerationProgressAsync()) (or equivalent lambda) to assert no exception is thrown while calling SimpleGenerationReporter.ReportSingleFileGenerationProgressAsync.
🤖 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.
Nitpick comments:
In `@src/Refitter.Tests/FileWriterTests.cs`:
- Around line 8-9: The TestRoot constant in FileWriterTests creates a shared
temp directory that is never cleaned up; update the test class (FileWriterTests)
to either perform one-time cleanup of TestRoot in a test fixture setup/teardown
(e.g., remove leftover GUID subdirectories under the directory referenced by
TestRoot) or stop using a shared TestRoot and instead generate per-test unique
temp directories using Path.GetTempPath() + Guid.NewGuid() where the tests
currently reference TestRoot; ensure the chosen approach deletes created
directories after tests complete to avoid orphaned temp folders.
In `@src/Refitter.Tests/SimpleGenerationReporterTests.cs`:
- Around line 33-37: The test currently awaits
ReportSingleFileGenerationProgressAsync directly; change it to use NUnit's async
"does not throw" pattern by wrapping the call in Assert.DoesNotThrowAsync so the
test method ReportSingleFileGenerationProgressAsync_Does_Not_Throw uses
Assert.DoesNotThrowAsync(() => new
SimpleGenerationReporter().ReportSingleFileGenerationProgressAsync()) (or
equivalent lambda) to assert no exception is thrown while calling
SimpleGenerationReporter.ReportSingleFileGenerationProgressAsync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed1d8d2b-6d6d-4625-9fc4-0351861a514e
📒 Files selected for processing (4)
src/Refitter.Tests/FileWriterTests.cssrc/Refitter.Tests/OpenApiStatisticsFormatterTests.cssrc/Refitter.Tests/SettingsFile/RefitterSettingsLoaderTests.cssrc/Refitter.Tests/SimpleGenerationReporterTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Refitter.Tests/SettingsFile/RefitterSettingsLoaderTests.cs
Description:
Refactors
GenerateCommandinto focused, testable modules and moves shared.refittersettings loading and relative path resolution intoRefitter.Core.This pull request now also includes a follow-up fix for the refactor’s Build workflow regression. The source generator package-reference tests now serialize the helper that builds and packs
Refitter.SourceGenerator, preventing concurrentnet8.0andnet10.0test runs from contending on the same build outputs.Included changes:
RefitterSettingsLoaderso CLI and Source Generator share one implementation for settings-file loading and relative spec path resolutionGenerateCommandoutput pipeline into focused modules:OutputPlannerFileWriterIGenerationReporterwith simple and rich implementationsOpenApiStatisticsFormatterGenerateCommandfrom ~1085 lines to ~430 lines.github/copilot-instructions.mdto require frequent small commits and prohibit co-author trailers for agentic workSourceGeneratorPackageReferenceTestsby serializing package build/pack execution across concurrent test processesVerification performed:
dotnet build -c Release src/Refitter.SourceGenerator.Tests/Refitter.SourceGenerator.Tests.csprojnet8.0andnet10.0source-generator test runs passedExample OpenAPI Specifications:
Not applicable for this refactor and test-stabilization pull request.Example generated Refit interface
// Not applicable for this refactor and test-stabilization pull request.Summary by CodeRabbit
New Features
Tests