[tcgc] client initialization and accessor enhancement #2027
Conversation
tadelesh
requested review from
lmazuel,
m-nash,
iscai-msft,
srnagar,
joheredi,
bterlson,
markcowl,
allenjzhang and
timotheeguerin
as code owners
|
All changed packages have been documented.
Show changes
|
tadelesh
changed the title
|
You can try these changes here
|
| initialization: SdkInitializationType; | ||
| clientInitialization: SdkClientInitializationMethod; |
There was a problem hiding this comment.
It is a bit strange that SdkClientInitializationMethod is here, while SdkClientAccessor<> appears in methods?
If this SdkClientInitializationMethod is something very special, do we need a method type?
There was a problem hiding this comment.
i also want to move client assessor method out of the method list. just tbd.
There was a problem hiding this comment.
are we doing the breaking change for initialization -> clientInitialization so we are the same as the decorator? I'm not 100% sure we need to make this change, but it's not a hill I'm going to die on
tadelesh
force-pushed
the
client-initialization
branch
from
c37f03e to
cc35540
Compare
|
two things to be discussed:
|
| | ------- | ---------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | ||
| | options | `Model` | | | ||
| | scope | `valueof string` | The language scope you want this decorator to apply to. If not specified, will apply to all language emitters.<br />You can use "!" to specify negation such as "!(java, python)" or "!java, !python". | | ||
| | Name | Type | Description | |
There was a problem hiding this comment.
@tadelesh for this design have other redmond folks signed off on it? I think it would be great to get others to look at the doc. I'll send it to DPG redmond sync to see what people think of the changes to the @clientInitialization decorator
| /** | ||
| * Determines the accessibility of the client accessor method. `Access.public` means client could be got from parent client. | ||
| */ | ||
| accessorAccess?: EnumMember; |
There was a problem hiding this comment.
i don't think this name is very clear, but I don't have a great suggestion. Accessible from parent? I'm just not sure that a spec author will have an understanding of "accessor" like us tcgc authors do
| }) | ||
| : undefined, | ||
| access: options.properties.get("access") | ||
| ? options.properties.get("access").type.value |
There was a problem hiding this comment.
nit: I think you can do options.properties.get('access')?.type?.value || undefined, or maybe you don't even need || undefined
| initialization: SdkInitializationType; | ||
| clientInitialization: SdkClientInitializationMethod; |
There was a problem hiding this comment.
are we doing the breaking change for initialization -> clientInitialization so we are the same as the decorator? I'm not 100% sure we need to make this change, but it's not a hill I'm going to die on
| clientParams.push(param); | ||
| } | ||
| } | ||
| const name = `init${client.kind === "SdkClient" ? client.name : getLibraryName(context, client.type)}`; |
There was a problem hiding this comment.
since the name doesn't super matter, should we just call it "init"? I think in terms of conflicts, since each one will be for a separate client, it should be fine
| } | ||
| return diagnostics.wrap(retval); | ||
| } | ||
|
|
||
| function createClientAccessorMethod<TServiceOperation extends SdkServiceOperation>( |
There was a problem hiding this comment.
just to clarify, we still have client accessor methods, denoting the relationship between a parent and child client. At the same time, instead of treating the initialization information in tcgc as a pure model, we are looking at it more like another operation. I like this way of thinking, I'm just trying to make sure I'm reading this correctly
| 1. Change `@clientInitialization` decorator and add `clientInitialization` property to `SdkClientType` | ||
| - Change `@clientInitialization` decorator's `options` parameter to `ClientInitializationOptions` type to accept `access` and `accessorAccess` setting. | ||
| - Add `clientInitialization` property to `SdkClientType`. It indicate the initialization method's signature, including the parameter list and access. | ||
| - When `access` set to `Access.public`, it means this client could be initialized separately, while `Access.internal` could not. Default to `Access.public` for client and `Access.internal` for sub client. |
There was a problem hiding this comment.
can you make clear that this is the access for the initialization property?
With this design, there seem to be 3 accesses associated with a client.
- The access of the
clientitself. This can't be toggled rn, it's alwayspublic - The access of the client's init. If
public, can directly init, else you can not - The access of the client's accessor. If
public, you can create this client from its parent. Else, not
| The `InnerGroup`'s `initialization` model's properties contains a property named `blobName`. The method is `public`. | ||
| The method `upload` no longer has `blobName` parameter, its corresponding operation's parameter `blobName` is mapped to the `blobName` property of `InnerGroup`'s `initialization` model. | ||
|
|
||
| 2. Move client accessor method out of the normal methods list of client and add a new `clientAccessor` property to put it. |
There was a problem hiding this comment.
will the clientAccessor be on the parent client or the child client? Off the top of my head, we want to know a client's subclients, so I think it would be better to have this on the parent
|
|
||
| 3. Consolidate `@client` and `@operationGroup` | ||
| - Deprecate decorator `@operationGroup` and `SdkOperationGroup` type. | ||
| - Current explicitly `@operationGroup` could be migrated to `@client`. If `@client` is nested, then it is a sub-client, will follow previous operation group default logic. |
There was a problem hiding this comment.
when we move @operationGroup to @client, we will move them to a client with init access internal and accessor access public, right?
@clientInitializationdecorator'soptionsparameter toClientInitializationOptionstype. The options now could set the client initialization method's access and client accessor's access. This is a behavior breaking change for this decorator. All specs that use this decorator should change from@clientInitialization(CustomizedOption)to `@clientInitialization({parameters: CustomizedOption})getClientInitializationnow returnClientInitializationOptionstypes. It includes the info of the client initialization parameter list, the client initialization access value and the client accessor access value.clientInitializationproperty toSdkClientType. It indicate the initialization method's signature, including the parameter list and access, etc.Related design could be seen in this gist: https://gist.github.com/tadelesh/bb1b04501d25dd1f19bc5b164fb79810.
Also resolve: #1696, #1702, #1707, #1715, #1518