Skip empty clients #5652
Skip empty clients #5652
Conversation
microsoft-github-policy-service
bot
added
the
emitter:client:csharp
|
API change check API changes are not detected in this pull request. |
| } | ||
| } | ||
|
|
||
| if (client.CustomCodeView?.Methods.Count > 0) |
There was a problem hiding this comment.
Checking for the Methods isn't enough because the custom code may be using Properties, Fields, or even just doing something in the constructor. We should simply check whether there is custom code or not, and not worry about the individual members.
There was a problem hiding this comment.
So, it seems like custom code has a different criteria that if there is any custom code exists we should not skip the client?
Because for client and sub-client, we only check the methods.
|
|
||
| foreach (var subclient in client.SubClients) | ||
| { | ||
| if (subclient.Methods.Count > 0) |
There was a problem hiding this comment.
Checking for the Methods for the subclient isn't enough - we either need to check for the operations on the input client or the methods on the client and its REST client. I think adding an internal property like HasOperations or HasMethods to ClientProvider should be added to encapsulate this logic.
There was a problem hiding this comment.
My initial thought is that, we should have the same criteria for client, sub-client and custom code, which is any of them should have some methods.
For clientProvider, if there is no methods which means there is no operation or helper methods. Given RestClientProvider only contains methods of CreateRequestMethod, we should not need to check RestClientProvider.
Same thing for subClient, which is also a clientProvider.
For custom code, I don't understand why user would like to have a custom property or field to hold the empty client.
Do you have a use case for this?
| { | ||
| if (subclient.Methods.Count > 0) | ||
| { | ||
| return true; |
There was a problem hiding this comment.
We need to also check to see if the subclients have custom code defined.
There was a problem hiding this comment.
added the check of custom code but only check methods of custom code for now.
| @@ -63,6 +67,54 @@ public void SetUp() | |||
| clientPipelineApi: TestClientPipelineApi.Instance); | |||
| } | |||
|
|
|||
| [Test] | |||
| public void TestEmptyClient() | |||
There was a problem hiding this comment.
We need a test that validates the behavior when custom code is added for the client and for a sub client. See https://github.com/microsoft/typespec/blob/main/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/ClientProviders/ClientProviderCustomizationTests.cs for examples of custom code tests. Probably the custom code tests should go in that file.
There was a problem hiding this comment.
Added tests, but only for custom code with methods for now
Based on the e2e experiment in #5651, the user can implement an empty client in TypeSpec and MGC still generates it.
So, this is not a specific case for Azure plugin only, and then we should skip the empty client generation in MGC as a general behavior.
The criteria of an empty client:
Resolves #5624