Fix multi-file output directory validation#1172
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMulti-file output handling now validates directory-style paths in CLI and settings-file flows, with updated docs, help text, and tests. Several core enum and utility files were also reformatted to file-scoped namespaces, and one test file’s using order changed. ChangesMulti-file output validation
Namespace formatting cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
christianhelle
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Refitter/Settings.cs (1)
38-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd XML docs to the updated public CLI settings members.
These changed public properties still rely only on
[Description]. Please add/// <summary>docs so the public API stays consistent with the C# guideline for surfaced members.As per coding guidelines, "Include XML documentation for public APIs in C# code".
Also applies to: 129-139
🤖 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/Settings.cs` around lines 38 - 45, The public CLI settings properties in Settings are currently documented only with [Description], so add XML <summary> comments for the updated surfaced members to keep the public API consistent with C# guidelines. Update the relevant properties in the settings class, including OutputPath and the other affected public CLI option members referenced by the change, and ensure each has a clear XML doc summary matching its existing description.Source: Coding guidelines
src/Refitter.Tests/GenerateCommandTests.cs (1)
45-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd the required scenario coverage for this feature as well.
These validator-level tests are good regressions, but they do not satisfy the repo rule for new OpenAPI features to also have
Refitter.Tests.Scenarioscoverage with generate → assert → build, including both OpenAPI 2.0 and 3.0 specs. That extra layer is what will catch end-to-end path handling regressions beyond the validator result string.As per coding guidelines, "New features must have unit tests in the
Refitter.Tests.Scenariosnamespace following the pattern: spec string → generate → assert → build" and "Test with both OpenAPI 2.0 and 3.0 specifications when working with OpenAPI specifications".🤖 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/GenerateCommandTests.cs` around lines 45 - 127, The new output-path validation behavior also needs end-to-end scenario coverage, not just SettingsValidator unit tests. Add Refitter.Tests.Scenarios tests that follow the existing generate → assert → build pattern and verify both single-file and multiple-files output handling. Use the scenario helpers around the generate command and assert the produced files for both OpenAPI 2.0 and OpenAPI 3.0 specs. Place the new coverage alongside the existing scenario tests so regressions in SettingsValidator and the generation flow are caught together.Source: Coding guidelines
src/Refitter/SettingsValidator.cs (1)
218-235: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated CLI
--outputoverride check across both validators.Lines 218-226 and 248-256 are identical. Extracting a shared helper (e.g.
TryGetCliOutputOverrideError(settings, outputPathSpecified)) would remove the duplication and keep the error wording in sync.Also applies to: 248-266
🤖 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/SettingsValidator.cs` around lines 218 - 235, The CLI `--output` override validation is duplicated in both validators, so extract the repeated `HasExplicitCliOutputOverride` and `IsFileLikePath` checks into a shared helper such as `TryGetCliOutputOverrideError` in `SettingsValidator`. Have both validation paths call that helper and return its `GetCliMultiFilePathValidationError` result so the `--output` wording stays consistent in one place and the duplicate blocks are removed.
🤖 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/GenerateCommand.cs`:
- Around line 32-34: The output path detection in GenerateCommand is missing the
combined argument form, so explicit values like --output=... are treated as
unspecified. Update the context.Arguments check in GenerateCommand to recognize
both the plain flags and the prefix forms by using the same outputPathSpecified
logic with StartsWith for --output= and -o= alongside the existing Equals
checks.
In `@src/Refitter/SettingsValidator.cs`:
- Around line 358-369: IsFileLikePath is too broad because it treats any path
with a non-empty extension as a file, which incorrectly rejects valid output
directories like Generated.Api or v1.0. Update SettingsValidator.IsFileLikePath
to only return true for known source/config file extensions instead of using
Path.GetExtension generically. Keep the existing normalization and null/empty
checks, but replace the extension heuristic with a fixed allowlist of file types
used by Refitter settings and outputs.
---
Nitpick comments:
In `@src/Refitter.Tests/GenerateCommandTests.cs`:
- Around line 45-127: The new output-path validation behavior also needs
end-to-end scenario coverage, not just SettingsValidator unit tests. Add
Refitter.Tests.Scenarios tests that follow the existing generate → assert →
build pattern and verify both single-file and multiple-files output handling.
Use the scenario helpers around the generate command and assert the produced
files for both OpenAPI 2.0 and OpenAPI 3.0 specs. Place the new coverage
alongside the existing scenario tests so regressions in SettingsValidator and
the generation flow are caught together.
In `@src/Refitter/Settings.cs`:
- Around line 38-45: The public CLI settings properties in Settings are
currently documented only with [Description], so add XML <summary> comments for
the updated surfaced members to keep the public API consistent with C#
guidelines. Update the relevant properties in the settings class, including
OutputPath and the other affected public CLI option members referenced by the
change, and ensure each has a clear XML doc summary matching its existing
description.
In `@src/Refitter/SettingsValidator.cs`:
- Around line 218-235: The CLI `--output` override validation is duplicated in
both validators, so extract the repeated `HasExplicitCliOutputOverride` and
`IsFileLikePath` checks into a shared helper such as
`TryGetCliOutputOverrideError` in `SettingsValidator`. Have both validation
paths call that helper and return its `GetCliMultiFilePathValidationError`
result so the `--output` wording stays consistent in one place and the duplicate
blocks are removed.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b832f1cd-88f6-4a4d-84da-3d1c605b488a
📒 Files selected for processing (13)
README.mdsrc/Refitter.Core/Settings/CacheProviderType.cssrc/Refitter.Core/Settings/MappingProviderType.cssrc/Refitter.Core/Utilities/VersionHelper.cssrc/Refitter.Tests/GenerateCommandTests.cssrc/Refitter.Tests/OpenApi/OpenApiValidatorTests.cssrc/Refitter.Tests/SettingsFile/SettingsValidatorTests.cssrc/Refitter/GenerateCommand.cssrc/Refitter/README.mdsrc/Refitter/Settings.cssrc/Refitter/SettingsValidator.cstest/smoke-tests.battest/smoke-tests.ps1
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1172 +/- ##
=======================================
Coverage 94.82% 94.82%
=======================================
Files 80 80
Lines 3553 3557 +4
=======================================
+ Hits 3369 3373 +4
Misses 75 75
Partials 109 109
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:
|
|
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 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
1 participant
Description:
.refitterdirectory settingsCloses #1169
Example OpenAPI Specifications:
N/A
Example generated Refit interface
N/A
Validation:
dotnet build -c Release src/Refitter.slnxdotnet test --solution src/Refitter.slnx -c Releasedotnet format --verify-no-changes src/Refitter.slnxSummary by CodeRabbit
--outputand--contracts-output, including clearer handling of when these values look like file paths vs directory paths..refitterformat docs with multi-file directory-path requirements and clarified--outputvs--contracts-outputbehavior.