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.

Sign up for GitHub

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

Jump to bottom

[tcgc] add @clientInitialization decorator #1398

Merged
merged 30 commits into from
Sep 3, 2024
Merged

[tcgc] add @clientInitialization decorator #1398

merged 30 commits into from
Sep 3, 2024

Conversation

iscai-msft
Copy link
Contributor

fixes #914

@azure-sdk
Copy link
Collaborator

azure-sdk commented Aug 19, 2024 •
edited
Loading

All changed packages have been documented.

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

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

add @clientInitialization decorator

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

tadelesh
tadelesh reviewed Aug 27, 2024
View reviewed changes
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.

three concerns:

  1. can we have a test with @client and @operationGroup?
  2. i'm wondering if we need to add a lint to disallow promoting method param to client if it is not in all client's operation?
  3. with this decorator, is it possible to remove the hard code for promoting subscriptionId cause it is not already client parameter in brownfield?

@iscai-msft
Copy link
Contributor Author

three concerns:

  1. can we have a test with @client and @operationGroup?
  2. i'm wondering if we need to add a lint to disallow promoting method param to client if it is not in all client's operation?
  3. with this decorator, is it possible to remove the hard code for promoting subscriptionId cause it is not already client parameter in brownfield?
  1. Yep, let me add this test
  2. I think this is a fair linter-rule to add for unbranded, but it's complicated because of ARM's behavior. Currently the list operations don't need subscriptionId but all ARM sdks still have subscriptionId as a client-level param
  3. Yeah that is down the line, where ARM can auto-apply the @clientInitialization decorators for subscriptionId

chunyu3
chunyu3 reviewed Aug 28, 2024
View reviewed changes
Copy link
Member

@chunyu3 chunyu3 left a comment

Choose a reason for hiding this comment

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

When the parameter blobName is bumped to client-level, there will no blobName in method (SdkMethod), but it will still in Operation (method.Operation). Right?
But we need to know that blobName is bumped to client-level, so that when we handle this parameter we will find it in Client. It is better to have a flag to indicate that the operation parameter is moved to client, such as Location="client"

@iscai-msft
Copy link
Contributor Author

@chunyu3 it will have property .onClient set to true

weidongxu-microsoft
weidongxu-microsoft approved these changes Aug 30, 2024
View reviewed changes
Copy link
Member

@weidongxu-microsoft weidongxu-microsoft left a comment

Choose a reason for hiding this comment

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

LGTM, having a few query though.

docs/libraries/typespec-client-generator-core/reference/decorators.md Show resolved Hide resolved
packages/typespec-client-generator-core/test/decorators.test.ts Outdated Show resolved Hide resolved
tadelesh
tadelesh reviewed Sep 2, 2024
View reviewed changes
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.

can we add a test of redefining the client hierarchy in client.tsp and then add @clientInitialization to the new defined client? it seems will work well from the code logic, but i'm not sure if there is any corner case.

packages/typespec-client-generator-core/generated-defs/Azure.ClientGenerator.Core.ts Show resolved Hide resolved
packages/typespec-client-generator-core/src/decorators.ts Outdated Show resolved Hide resolved
packages/typespec-client-generator-core/src/decorators.ts Outdated Show resolved Hide resolved
packages/typespec-client-generator-core/src/decorators.ts Outdated Show resolved Hide resolved
packages/typespec-client-generator-core/src/decorators.ts Outdated Show resolved Hide resolved
packages/typespec-client-generator-core/src/http.ts Show resolved Hide resolved
tadelesh
tadelesh approved these changes Sep 2, 2024
View reviewed changes
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.

overall lgtm. one thing is could you also check if we could solve this: #1391 in this pr?

@MaryGao MaryGao mentioned this pull request Sep 3, 2024
Merged
@iscai-msft iscai-msft added this pull request to the merge queue Sep 3, 2024
Merged via the queue into main with commit 07959d0 Sep 3, 2024
22 checks passed
@iscai-msft iscai-msft deleted the add_client_initialization branch September 3, 2024 21:29
@haolingdong-msft haolingdong-msft mentioned this pull request Sep 4, 2024
Closed
markcowl pushed a commit to markcowl/typespec-azure that referenced this pull request Sep 7, 2024
@iscai-msft @markcowl
[tcgc] add @clientInitialization decorator (Azure#1398)
3e36dd9 
fixes Azure#914

---------

Co-authored-by: iscai-msft <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haolingdong-msft haolingdong-msft haolingdong-msft left review comments

@weidongxu-microsoft weidongxu-microsoft weidongxu-microsoft approved these changes

@chunyu3 chunyu3 chunyu3 left review comments

@tadelesh tadelesh tadelesh approved these changes

@lmazuel lmazuel Awaiting requested review from lmazuel lmazuel is a code owner

@m-nash m-nash Awaiting requested review from m-nash m-nash is a code owner

@srnagar srnagar Awaiting requested review from srnagar srnagar is a code owner

@joheredi joheredi Awaiting requested review from joheredi joheredi is a code owner

@bterlson bterlson Awaiting requested review from bterlson bterlson is a code owner

@markcowl markcowl Awaiting requested review from markcowl markcowl is a code owner

@allenjzhang allenjzhang Awaiting requested review from allenjzhang allenjzhang is a code owner

@timotheeguerin timotheeguerin Awaiting requested review from timotheeguerin timotheeguerin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TCGC: @methodLocation decorator
6 participants
@iscai-msft @azure-sdk @tadelesh @chunyu3 @weidongxu-microsoft @haolingdong-msft