-
Notifications
You must be signed in to change notification settings - Fork 45
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] do not replace operation with @override
when listOperationsInOperationGroup
#1858
Conversation
All changed packages have been documented.
Show changes
|
You can try these changes here
|
@@ -586,7 +586,7 @@ export function listOperationsInOperationGroup( | |||
for (const op of current.operations.values()) { | |||
// Skip templated operations | |||
if (!isTemplateDeclarationOrInstance(op)) { | |||
operations.push(getOverriddenClientMethod(context, op) ?? op); | |||
operations.push(op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the decided way to proceed until every language can fully support overrides? I feel it would be better to still return these instead of the original, and we could tell emitter authors that it's overridden, and we can point back to the original operation as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listOperationsInOperationGroup
is a decorator helper, which return the raw typespec operation. if you return the override one, then http payload will be changed. it is a bug, not a design change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh i see, approving
- "@azure-tools/typespec-client-generator-core" | ||
--- | ||
|
||
do not replace operation with `@override` when `listOperationsInOperationGroup` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it only affect listOperationsInOperationGroup
?
Java emitter currently list SdkServiceMethod
under client.methods
.
From the context, this seems also should not be the @override
operation -- SdkServiceMethod
should have the signature of the override operation in SdkServiceMethod
, while having the signature of the original/overrided operation in SdkServiceOperation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is just a bug for previous @override
impl. for SdkServiceMethod
it will use the @override
operation, for SdkServiceOperation
it will use the original operation, these two logic is right for the impl.
fix: #1857