Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tcgc] add client initialization usage #1664

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

iscai-msft
Copy link
Contributor

@iscai-msft iscai-msft commented Oct 10, 2024

fixes #1518, #1707

@iscai-msft iscai-msft changed the title Tcgc/add client initialization usage [tcgc] add client initialization usage Oct 10, 2024
@azure-sdk
Copy link
Collaborator

azure-sdk commented Oct 10, 2024

All changed packages have been documented.

  • @azure-tools/typespec-client-generator-core
Show changes

@azure-tools/typespec-client-generator-core - breaking ✏️

Make SdkInitializationType have an access property to denote if publicly instantiable, and a model property to show the client options model

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

export interface SdkInitializationType extends SdkModelType {
properties: SdkParameter[];
export interface SdkInitializationType {
access: AccessFlags;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've realized we can't modify the .access on our existing SdkInitializationType to denote whether a client is publicly or internally initialized. There is both the context that the client options model is used, and the client options model itself. In this case, the context that the client options model is used has access that depends on whether it's a client or og. However, that doesn't change the actual access of the client options model itself.

This is a breaking change, so want to call this out and will call it out more in our sync

Copy link
Member

@tadelesh tadelesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments, and i think for default access settings, we shall consider: #1702.

packages/typespec-client-generator-core/src/types.ts Outdated Show resolved Hide resolved
model: Model,
): SdkInitializationType {
// convert to SDK type
const existingModel = context.modelsMap?.get(model) as SdkModelType<SdkParameter> | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we allow to reuse model in operation as a client initialization model? if so, can we have a test? if not, we could return directly and shall have some lint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-nash since c# appears to be the main sdk using client initialization model generation, is there anything precluding us from having a model be used in both scenarios?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ClientInitialization usage for models
3 participants