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

typespec-autorest does not honor @clientName #442

Closed
tadelesh opened this issue Mar 19, 2024 · 12 comments · Fixed by #1455
Closed

typespec-autorest does not honor @clientName #442

tadelesh opened this issue Mar 19, 2024 · 12 comments · Fixed by #1455
Assignees
Labels
design:accepted Proposal for design has been discussed and accepted. emitter:autorest Issues for @azure-tools/typespec-autorest emitter feature New feature or request triaged:core

Comments

@timotheeguerin
Copy link
Member

This was partially by design that the only thing it support was what it supported with @projectedName and we just wanted parity. However it would probably make sense to expand the usage.

@markcowl
Copy link
Member

+1, we should either add x-ms-client-name or change the schema name, since it isn't actually part of the API

@tadelesh
Copy link
Member Author

Previously, we have @projectedName("client", "xxx") to rename the type in swagger with x-ms-client-name. So my idea is do the same thing for @clientName.

@timotheeguerin
Copy link
Member

timotheeguerin commented Mar 20, 2024

That didn't work on the model name See playground, that is my point, we only kept parity.

but I don’t have anything against adding more cases

@tadelesh
Copy link
Member Author

That didn't work on the model name See playground, that is my point, we only kept parity.

but I don’t have anything against adding more cases

Oh, I see. Thanks for explanation.

@markcowl then we should add both @extension("x-ms-client-name", "xxx") and @clientName("xxx") to keep client generated from swagger and tsp to be the same.

@timotheeguerin
Copy link
Member

timotheeguerin commented Mar 20, 2024

Why do you need to add x-ms-client-name on the model, we should just change the definition name

@tadelesh
Copy link
Member Author

Oh. I see. Thanks for the explanation.

@tadelesh tadelesh closed this as not planned Won't fix, can't repro, duplicate, stale Mar 21, 2024
@tadelesh
Copy link
Member Author

tadelesh commented Mar 21, 2024

For model/enum/interface/union renaming, since name is not used in wire, we should just rename the name. For parameters/properties renaming, we should use @clientName.

@timotheeguerin
Copy link
Member

Confused here @tadelesh, don’t we need this? The point was more that autorest should rename the definition not add x-Ms-client-name

@tadelesh
Copy link
Member Author

tadelesh commented Mar 21, 2024

Oh, I got your point. My view is from convert swagger to TypeSpec, we don't need to add @clientName, instead just rename the name for that not impact wire when conversion.

From autorest emitter view, the requirements are:

  1. for name that is used in wire, @clientName emit with x-ms-client-name
  2. for name that is not used in wire, @clientName change the swagger definition name directly

@tadelesh tadelesh reopened this Mar 21, 2024
@markcowl markcowl added the design:needed A design request has been raised that needs a proposal label Mar 22, 2024
@markcowl markcowl added this to the [2024] April milestone Mar 22, 2024
@markcowl markcowl self-assigned this Mar 27, 2024
@markcowl markcowl modified the milestones: [2024] April, [2024] May Apr 6, 2024
@markcowl markcowl modified the milestones: [2024] May, [2024] June May 6, 2024
@markcowl markcowl removed their assignment May 10, 2024
@markcowl markcowl modified the milestones: [2024] June, [2024] July Jun 17, 2024
@markcowl markcowl modified the milestones: [2024] July, [2024] August Jul 19, 2024
@markcowl
Copy link
Member

The consensus is to update typespec-autorest to honor @clientName in more cases:

For Models:
@clientName should change the schema name
For Enums and Unions
@clientName should change the schema name and the x-ms-enum name field

@markcowl markcowl added design:accepted Proposal for design has been discussed and accepted. and removed design:needed A design request has been raised that needs a proposal labels Aug 12, 2024
@markcowl markcowl removed this from the [2024] September milestone Aug 12, 2024
@markcowl markcowl assigned markcowl and unassigned allenjzhang and markcowl Aug 12, 2024
@markcowl markcowl added emitter:autorest Issues for @azure-tools/typespec-autorest emitter feature New feature or request labels Aug 13, 2024
@timotheeguerin
Copy link
Member

other thing it should change is the body parameter name and x-ms-client-name for path and query

@markcowl markcowl added this to the Backlog milestone Aug 21, 2024
@markcowl markcowl added lib:azure-core Issues for @azure-tools/typespec-azure-core library triaged:core and removed lib:azure-core Issues for @azure-tools/typespec-azure-core library labels Aug 21, 2024
@timotheeguerin timotheeguerin self-assigned this Aug 30, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 30, 2024
fix [#442](#442)

`@clientName` is respected for:
- definition names
- enum values names
- parameters names(for body it replace the `name` for other it adds
`x-ms-client-name`)

Doing this now as this will help getting rid of some use of `@extension`

This makes this doc accurate now
https://azure.github.io/typespec-azure/docs/next/migrate-swagger/faq/breakingchange#createorupdate-put-apis
as well
markcowl pushed a commit to markcowl/typespec-azure that referenced this issue Sep 7, 2024
fix [Azure#442](Azure#442)

`@clientName` is respected for:
- definition names
- enum values names
- parameters names(for body it replace the `name` for other it adds
`x-ms-client-name`)

Doing this now as this will help getting rid of some use of `@extension`

This makes this doc accurate now
https://azure.github.io/typespec-azure/docs/next/migrate-swagger/faq/breakingchange#createorupdate-put-apis
as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design:accepted Proposal for design has been discussed and accepted. emitter:autorest Issues for @azure-tools/typespec-autorest emitter feature New feature or request triaged:core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants