Refactor parameter extraction into a ParameterListBuilder module#1180
Merged
Conversation
…eep ParameterListBuilder
christianhelle
added
enhancement
Contributor
📝 WalkthroughWalkthroughThis PR refactors parameter extraction in Refitter.Core, removing ChangesParameter extraction refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (8)
src/Refitter.Core/ParameterExtraction/ParameterDefaultValueFormatter.cs (2)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
varper repo convention.As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent per .editorconfig)".Also applies to: 50-50
🤖 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/ParameterExtraction/ParameterDefaultValueFormatter.cs` around lines 29 - 30, Replace the inferred local declaration in ParameterDefaultValueFormatter with an explicit string type to match the repo’s C# style convention. Update the numericString assignment in the default value formatting logic to use string rather than var, keeping the existing IFormattable/CultureInfo.InvariantCulture behavior unchanged.Source: Coding guidelines
11-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated
"default"literal into a constant.SonarCloud flags 6 occurrences of the
"default"literal in this file; consolidating reduces typo risk if behavior ever needs to change.♻️ Suggested fix
internal static class ParameterDefaultValueFormatter { + private const string DefaultLiteral = "default"; + public static bool IsNumericType(string type)then replace each
"default"return withDefaultLiteral.🤖 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/ParameterExtraction/ParameterDefaultValueFormatter.cs` around lines 11 - 79, The ParameterDefaultValueFormatter methods repeat the same "default" literal in several return paths, so extract it into a shared constant and use that everywhere instead. Add a single constant near the existing helpers in ParameterDefaultValueFormatter, then update FormatNumericValue, FormatDefaultValue, and GetDefaultValueForParameter to return that constant instead of the inline string.Source: Linters/SAST tools
src/Refitter.Core/ParameterExtraction/ParameterAttributeFormatter.cs (2)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
varper repo convention.As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent per .editorconfig)".🤖 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/ParameterExtraction/ParameterAttributeFormatter.cs` around lines 34 - 35, The ParameterAttributeFormatter logic uses var against the repo’s C# style convention. Update the local declarations in the formatter path, especially the values assigned in ParameterAttributeFormatter, to use explicit types instead of var, keeping the existing behavior unchanged while matching the editorconfig rule.Source: Coding guidelines
10-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the two
GetAliasAsAttributeoverloads.Both overloads implement the same comparison logic; the first can delegate to the second.
♻️ Suggested consolidation
public static string GetAliasAsAttribute(CSharpParameterModel parameterModel) => - string.Equals(parameterModel.Name, parameterModel.VariableName) - ? string.Empty - : $"AliasAs(\"{ParameterNaming.EscapeString(parameterModel.Name)}\")"; + GetAliasAsAttribute(parameterModel.Name, parameterModel.VariableName);🤖 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/ParameterExtraction/ParameterAttributeFormatter.cs` around lines 10 - 18, The two GetAliasAsAttribute overloads in ParameterAttributeFormatter duplicate the same comparison and formatting logic. Update the CSharpParameterModel overload to delegate to the string-based GetAliasAsAttribute(originalName, variableName) overload using parameterModel.Name and parameterModel.VariableName, and keep the Ordinal comparison and AliasAs formatting in only one place.src/Refitter.Core/ParameterExtraction/ParameterTypeResolver.cs (2)
42-45: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse the
charoverload ofEndsWith.
EndsWith(string)performs a culture-aware comparison by default; for a single-character check,EndsWith(char)is faster and avoids ambiguity.⚡ Suggested fix
- if (settings.OptionalParameters && - !type.EndsWith("?") && + if (settings.OptionalParameters && + !type.EndsWith('?') && (parameterModel.IsNullable || parameterModel.IsOptional || !parameterModel.IsRequired))🤖 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/ParameterExtraction/ParameterTypeResolver.cs` around lines 42 - 45, The nullable-type suffix check in ParameterTypeResolver should use the char-based EndsWith overload instead of the string overload. Update the conditional in the parameter type formatting logic so the type check uses the single-character '?' overload alongside the existing OptionalParameters and parameterModel nullability/optionality conditions, keeping the same behavior while avoiding the culture-aware string comparison.Source: Linters/SAST tools
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
varper repo convention.As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent per .editorconfig)".Also applies to: 65-65, 91-91, 99-99
🤖 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/ParameterExtraction/ParameterTypeResolver.cs` at line 38, The ParameterTypeResolver code uses var in multiple spots against the repo’s C# style convention. Replace the inferred declarations in ParameterTypeResolver (including the TrimImportedNamespaces call result and the other flagged local variables in the same class) with explicit types so the code matches the editorconfig guidance.Source: Coding guidelines
src/Refitter.Core/ParameterExtraction/ParameterNaming.cs (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
varper repo convention.
csharp_style_var_* = false:silentin.editorconfigdiscouragesvar; use explicit types (StringBuilder,string) here.As per coding guidelines, "Discourage use of
varkeyword in C# (csharp_style_var_* = false:silent per .editorconfig)".Also applies to: 64-64
🤖 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/ParameterExtraction/ParameterNaming.cs` at line 18, The issue is use of `var` in `ParameterNaming`, which conflicts with the repo’s explicit-type convention. Update the local declarations in `ParameterNaming` to use concrete types instead of inferred types, including the `StringBuilder` initialization and the other `var` usage mentioned in the comment, so the implementation matches the `.editorconfig` rule and the surrounding C# style.Source: Coding guidelines
src/Refitter.Tests/Parameters/ParameterListBuilderTests.cs (1)
129-140: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReordering test doesn't actually exercise the reorder logic.
page(required) is declared beforelimit(optional) inRequiredAndOptionalQuerySpec, so the natural extraction order already places the optional parameter last. The assertion would pass even ifOptionalParameterReorderer.Reorderwere a no-op, so this test doesn't meaningfully verify reordering behavior. Declarelimitbeforepagein the spec so the reorderer actually has to move something.♻️ Proposed fix
private const string RequiredAndOptionalQuerySpec = """ openapi: 3.0.0 info: title: Test version: "1.0" paths: /test: get: operationId: getTest parameters: - - name: page - in: query - required: true - schema: - type: integer - name: limit in: query schema: type: integer + - name: page + in: query + required: true + schema: + type: integer responses: '200': description: Success """;🤖 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/Parameters/ParameterListBuilderTests.cs` around lines 129 - 140, The test in ParameterListBuilderTests for Build_Orders_Optional_Parameters_After_Required currently doesn’t verify OptionalParameterReorderer.Reorder because RequiredAndOptionalQuerySpec already yields the optional parameter last. Update the spec used by SetupAsync so the optional parameter (limit) is declared before the required one (page), then keep the assertion on ParameterListBuilder.Build to confirm the reorder logic actually moves optional parameters after required ones.
🤖 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.Core/ParameterExtraction/ParameterAttributeFormatter.cs`:
- Around line 34-35: The ParameterAttributeFormatter logic uses var against the
repo’s C# style convention. Update the local declarations in the formatter path,
especially the values assigned in ParameterAttributeFormatter, to use explicit
types instead of var, keeping the existing behavior unchanged while matching the
editorconfig rule.
- Around line 10-18: The two GetAliasAsAttribute overloads in
ParameterAttributeFormatter duplicate the same comparison and formatting logic.
Update the CSharpParameterModel overload to delegate to the string-based
GetAliasAsAttribute(originalName, variableName) overload using
parameterModel.Name and parameterModel.VariableName, and keep the Ordinal
comparison and AliasAs formatting in only one place.
In `@src/Refitter.Core/ParameterExtraction/ParameterDefaultValueFormatter.cs`:
- Around line 29-30: Replace the inferred local declaration in
ParameterDefaultValueFormatter with an explicit string type to match the repo’s
C# style convention. Update the numericString assignment in the default value
formatting logic to use string rather than var, keeping the existing
IFormattable/CultureInfo.InvariantCulture behavior unchanged.
- Around line 11-79: The ParameterDefaultValueFormatter methods repeat the same
"default" literal in several return paths, so extract it into a shared constant
and use that everywhere instead. Add a single constant near the existing helpers
in ParameterDefaultValueFormatter, then update FormatNumericValue,
FormatDefaultValue, and GetDefaultValueForParameter to return that constant
instead of the inline string.
In `@src/Refitter.Core/ParameterExtraction/ParameterNaming.cs`:
- Line 18: The issue is use of `var` in `ParameterNaming`, which conflicts with
the repo’s explicit-type convention. Update the local declarations in
`ParameterNaming` to use concrete types instead of inferred types, including the
`StringBuilder` initialization and the other `var` usage mentioned in the
comment, so the implementation matches the `.editorconfig` rule and the
surrounding C# style.
In `@src/Refitter.Core/ParameterExtraction/ParameterTypeResolver.cs`:
- Around line 42-45: The nullable-type suffix check in ParameterTypeResolver
should use the char-based EndsWith overload instead of the string overload.
Update the conditional in the parameter type formatting logic so the type check
uses the single-character '?' overload alongside the existing OptionalParameters
and parameterModel nullability/optionality conditions, keeping the same behavior
while avoiding the culture-aware string comparison.
- Line 38: The ParameterTypeResolver code uses var in multiple spots against the
repo’s C# style convention. Replace the inferred declarations in
ParameterTypeResolver (including the TrimImportedNamespaces call result and the
other flagged local variables in the same class) with explicit types so the code
matches the editorconfig guidance.
In `@src/Refitter.Tests/Parameters/ParameterListBuilderTests.cs`:
- Around line 129-140: The test in ParameterListBuilderTests for
Build_Orders_Optional_Parameters_After_Required currently doesn’t verify
OptionalParameterReorderer.Reorder because RequiredAndOptionalQuerySpec already
yields the optional parameter last. Update the spec used by SetupAsync so the
optional parameter (limit) is declared before the required one (page), then keep
the assertion on ParameterListBuilder.Build to confirm the reorder logic
actually moves optional parameters after required ones.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bcab9ef-4aac-4876-b2cb-02f25e9b99b7
📒 Files selected for processing (23)
CONTEXT.mdsrc/Refitter.Core/Generation/InterfaceGenerator.cssrc/Refitter.Core/Generation/MethodSignatureGenerator.cssrc/Refitter.Core/ParameterExtraction/BodyParameterExtractor.cssrc/Refitter.Core/ParameterExtraction/DynamicQuerystringParameterBuilder.cssrc/Refitter.Core/ParameterExtraction/FormParameterExtractor.cssrc/Refitter.Core/ParameterExtraction/HeaderParameterExtractor.cssrc/Refitter.Core/ParameterExtraction/IParameterExtractor.cssrc/Refitter.Core/ParameterExtraction/IParameterTypeExtractor.cssrc/Refitter.Core/ParameterExtraction/OptionalParameterReorderer.cssrc/Refitter.Core/ParameterExtraction/ParameterAggregator.cssrc/Refitter.Core/ParameterExtraction/ParameterAttributeFormatter.cssrc/Refitter.Core/ParameterExtraction/ParameterDefaultValueFormatter.cssrc/Refitter.Core/ParameterExtraction/ParameterList.cssrc/Refitter.Core/ParameterExtraction/ParameterListBuilder.cssrc/Refitter.Core/ParameterExtraction/ParameterNaming.cssrc/Refitter.Core/ParameterExtraction/ParameterShared.cssrc/Refitter.Core/ParameterExtraction/ParameterTypeResolver.cssrc/Refitter.Core/ParameterExtraction/QueryParameterExtractor.cssrc/Refitter.Core/ParameterExtraction/RouteParameterExtractor.cssrc/Refitter.Tests/MethodSignatureGeneratorTests.cssrc/Refitter.Tests/Parameters/ParameterExtractorPrivateCoverageTests.cssrc/Refitter.Tests/Parameters/ParameterListBuilderTests.cs
💤 Files with no reviewable changes (4)
- src/Refitter.Core/ParameterExtraction/IParameterExtractor.cs
- src/Refitter.Core/ParameterExtraction/ParameterAggregator.cs
- src/Refitter.Core/ParameterExtraction/ParameterShared.cs
- src/Refitter.Core/ParameterExtraction/IParameterTypeExtractor.cs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1180 +/- ##
==========================================
+ Coverage 94.83% 94.85% +0.01%
==========================================
Files 81 85 +4
Lines 3585 3578 -7
==========================================
- Hits 3400 3394 -6
Misses 76 76
+ Partials 109 108 -1
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:
|
christianhelle
changed the title
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
Deepens the parameter-extraction subsystem in
Refitter.Coreinto a single deep module,ParameterListBuilder, replacing a cluster of shallow modules and a 322-line static god-helper.Previously, building a generated Refit method's parameter list spanned
MethodSignatureGenerator→ParameterAggregator(IParameterExtractor) → fiveIParameterTypeExtractoradapters → the staticParameterShared, with the query path threading state through mutable properties and anoutparameter. Each seam had exactly one implementation and was never substituted (the tests constructed the concretes directly), so the indirection added no leverage.What changed
ParameterListBuilder(settings).Build(operationModel, operation, dynamicQuerystringParameterType) → ParameterList. Settings are injected once via the constructor and the result is a record (Parameters,DynamicQuerystringCode) — no moreoutparameter or mutableQueryParameterExtractorstate.IParameterExtractorandIParameterTypeExtractorinterfaces andParameterAggregator. The five per-kind extractors remain as internal collaborators.ParameterShared(322 lines) into four cohesive internal helpers:ParameterNaming,ParameterDefaultValueFormatter,ParameterAttributeFormatter,ParameterTypeResolver.AppendXmlDocCommentmoved next to its only caller.Tests
ParameterExtractorDeepCoverageTests,ParameterExtractorEdgeCaseTests) unchanged.ParameterListBuilderTestscovering the dynamic-querystring result, optional-parameter reordering, and the request-options / cancellation-token append through the newBuildinterface.dotnet formatclean.Notes
CONTEXT.mdseeding the Parameter list domain term.fix:commit corrects aMethodSignatureGeneratortest that had never actually exercised the dynamic-querystring path.Summary by CodeRabbit
New Features
Documentation
Tests