[http-client-csharp-mgmt] code refactoring #50205
Conversation
haiyuazhang
requested review from
JoshLove-msft,
m-nash,
jorgerangel-msft,
live1206 and
ArcturusZhang
as code owners
github-actions
bot
added
CodeGen
There was a problem hiding this comment.
Pull Request Overview
A refactoring PR for the HTTP client management generator, focusing on simplifying method signatures and updating return types.
- Introduces new extension methods on InputServiceMethod for handling long-running operations.
- Updates several methods to return IReadOnlyList instead of single statements.
- Refactors BuildOperationMethod and related methods in ResourceClientProvider to remove the isGeneric parameter and streamline try-catch logic.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Azure.Generator.Management/src/Utilities/InputMethodExtensions.cs | Adds helper extension methods to determine LRO properties and response types. |
| Azure.Generator.Management/src/Providers/ResourceCollectionClientProvider.cs | Refactors return types for building method statements and simplifies method call signatures. |
| Azure.Generator.Management/src/Providers/ResourceClientProvider.cs | Simplifies operation method building by removing the isGeneric parameter and restructuring internal try-catch and pipeline-processing logic. |
Comments suppressed due to low confidence (1)
eng/packages/http-client-csharp-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceClientProvider.cs:251
- The field '_clientDiagonosticsField' appears to have a typo. Consider renaming it to '_clientDiagnosticsField' for improved clarity and consistency.
UsingDeclare("scope", typeof(DiagnosticScope), _clientDiagonosticsField.Invoke(nameof(ClientDiagnostics.CreateScope), [Literal("${Name}.${signature.Name}")]), out var scopeVariable),
354e449 to
aab13f8
Compare
...ient-csharp-mgmt/generator/Azure.Generator.Management/src/Utilities/InputMethodExtensions.cs
Outdated
Show resolved
Hide resolved
...ient-csharp-mgmt/generator/Azure.Generator.Management/src/Utilities/InputMethodExtensions.cs
Outdated
Show resolved
Hide resolved
...ient-csharp-mgmt/generator/Azure.Generator.Management/src/Utilities/InputMethodExtensions.cs
Outdated
Show resolved
Hide resolved
...ent-csharp-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceClientProvider.cs
Outdated
Show resolved
Hide resolved
...ent-csharp-mgmt/generator/Azure.Generator.Management/src/Providers/ResourceClientProvider.cs
Outdated
Show resolved
Hide resolved
| @@ -233,7 +233,7 @@ protected override bool SkipMethodParameter(ParameterProvider parameter) | |||
| return ContextualParameters.Take(ContextualParameters.Count - 1).Any(p => p == parameter.Name); | |||
| } | |||
|
|
|||
| protected override MethodBodyStatement BuildReturnStatements(ValueExpression responseVariable, MethodSignature signature) | |||
| protected override IReadOnlyList<MethodBodyStatement> BuildReturnStatements(ValueExpression responseVariable, MethodSignature signature) | |||
| { | |||
| if (signature.Name == "GetIfExists" || signature.Name == "GetIfExistsAsync") | |||
There was a problem hiding this comment.
Can we refine BuildOperationMethodCore further?
It's odd to apply parts of the method body differently depending on method name.
Can we abstract something like ResourceOperationMethodProvider? Then we can have a different instance of it for each method.
haiyuazhang
force-pushed
the
haiyzhan/mgmt-generator/code_refactory
branch
from
4fc9a68 to
d293824
Compare
haiyuazhang
force-pushed
the
haiyzhan/mgmt-generator/code_refactory
branch
from
9702022 to
7e0e865
Compare
| var responseBodyCSharpType = method.GetResponseBodyType(); | ||
| CSharpType rawReturnType = method.GetOperationMethodRawReturnType(resourceClientCSharpType, resourceDataType); |
There was a problem hiding this comment.
so from below - this responseBodyCSharpType is only used to check if it is null.
the GetOperationMethodRawReturnType did not really use this value, instead, it construct this value again inside the method.
We have quite a few duplicated and confusing logic here.
There was a problem hiding this comment.
Also I kind of think that this isAsync is breaking up our core logic - we are having so many ternary operator just doing:
isAsync ? wrap typeof Task : originalType;
I think we could remove this isAsync parameter from this method, and wrap it by isAsync later by another method, such as an extension method to CSharpType WrapAsync(isAsync) which returns you the original type if isAsync is false, or wraps the Task type if isAsync is true.
| { | ||
| var returnValueExpression = responseVariable.Property("Value").NotEqual(Null); | ||
| return Return(Static(typeof(Response)).Invoke(nameof(Response.FromValue), returnValueExpression, responseVariable.Invoke("GetRawResponse"))); | ||
| return [Return(Static(typeof(Response)).Invoke(nameof(Response.FromValue), returnValueExpression, responseVariable.Invoke("GetRawResponse")))]; |
There was a problem hiding this comment.
nit: could we spread the elements into multiple lines so that the statements could be more clear?
Copilot Generated Pull Request Description is accurate.