Add support for client options generation #4019
Conversation
microsoft-github-policy-service
bot
added
the
emitter:client:csharp
|
API change check API changes are not detected in this pull request. |
jorgerangel-msft
force-pushed
the
client-options
branch
from
c6ce518
to
582602a
Compare
jorgerangel-msft
changed the title
jorgerangel-msft
requested review from
ArcturusZhang,
ShivangiReja and
nisha-bhatia
|
|
||
| public ClientOptionsProvider(InputClient inputClient) | ||
| public ClientOptionsProvider(InputClient inputClient, ClientProvider clientProvider) |
There was a problem hiding this comment.
Can we avoid passing in the ClientProvider?
TF should give you an idempotent answer if you asked for the ClientProvider vis the InputClient so should be safe to simply pass in the InputClient
There was a problem hiding this comment.
do we need to do the same for RestClientProvider ?
There was a problem hiding this comment.
I think we should do the same in RestClientProvider
There was a problem hiding this comment.
ah so making this change creates an issue which leads to a stack overflow. CreateClientCore will call the ClientProvider ctor if it can't find the provider in cache. ClientProvider now has 2 properties for 2 different providers RestClient and ClientOptions. If we use the TF in those providers to get the client, this will lead to a loop of instantiating ClientProvider and it will never be cached. I will keep this as is for now and open a follow up issue to find a solution for this unless there are concerns.
...harp/generator/Microsoft.Generator.CSharp.ClientModel/src/Providers/ClientOptionsProvider.cs
Outdated
Show resolved
Hide resolved
...harp/generator/Microsoft.Generator.CSharp.ClientModel/src/Providers/ClientOptionsProvider.cs
Outdated
Show resolved
Hide resolved
...harp/generator/Microsoft.Generator.CSharp.ClientModel/src/Providers/ClientOptionsProvider.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace Microsoft.Generator.CSharp.ClientModel.Primitives | ||
| { | ||
| internal sealed class ApiVersion |
There was a problem hiding this comment.
I am not sure we need this class, since this is almost a duplicate of the Enum types we already have. The ApiVersions are just a FixedEnumProvider.
There was a problem hiding this comment.
Hmm so convert the list of apiversions from the ns to a list of InputEnumTypes so we can use FixedEnumProvider?
| private readonly IReadOnlyList<ApiVersion> _apiVersions; | ||
| private IReadOnlyList<EnumTypeMember>? _members; | ||
|
|
||
| public ServiceVersionDefinition(ClientOptionsProvider declaringType) |
There was a problem hiding this comment.
any reason we can't use fixedenumprovider?
There was a problem hiding this comment.
That provider takes in an InputEnumType in the ctor. Are you suggesting I convert the api version string to an InputEnumType ?
There was a problem hiding this comment.
I think we cannot reuse FixedEnumProvider because it is internal in MGC project.
We could make it public and inherit it here because we want to add a declaring type provider here, and I think it does not make much sense we only change the definition of FixedEnumProvider to let it take a DeclaringTypeProvider but leave others unchanged
And another issue of reusing FixedEnumProvider would be the identifier name.
FixedEnumProvider is implemented as "it calls ToCleanName to get the member name", but here we want a name like V2024_03_01 (I explained how it is constructed here). We need to figure out a solution for this as well if we want to reuse.
I think it is not worth it to reuse if it takes too much changes, this definition is simple enough right now, reusing might make it more complicated.
packages/http-client-csharp/generator/Microsoft.Generator.CSharp/src/Providers/FieldProvider.cs
Outdated
Show resolved
Hide resolved
| f.Type.PropertyInitializationType, | ||
| f.Name.ToCleanName(), | ||
| new AutoPropertyBody(true), | ||
| backingField: f)); |
There was a problem hiding this comment.
I think we are using the backing field in the wrong way here. A property and its corresponding backing field MUST belong to the same type. You are associating a field that belongs to one type with a property that belongs to another.
We should find another way to do this and remove BackingField if this is the only usage for it.
There was a problem hiding this comment.
I wonder what kind of code this is generating? Could we write some comment here?
There was a problem hiding this comment.
I've removed this for now. Please have a look at the latest implementation.
...erator/TestProjects/Local/Unbranded-TypeSpec/src/Generated/UnbrandedTypeSpecClientOptions.cs
Show resolved
Hide resolved
...ttp-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Mocks/MockHelpers.cs
Outdated
Show resolved
Hide resolved
...harp/generator/Microsoft.Generator.CSharp.ClientModel/src/Providers/ClientOptionsProvider.cs
Outdated
Show resolved
Hide resolved
...harp/generator/Microsoft.Generator.CSharp.ClientModel/src/Providers/ClientOptionsProvider.cs
Outdated
Show resolved
Hide resolved
...p/generator/Microsoft.Generator.CSharp.ClientModel/src/Providers/ServiceVersionDefinition.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I think we should have some tests to validate this on the generated files
There are quite a few scenarios on ClientOptions and the service versions
fixes: #3688