-
Notifications
You must be signed in to change notification settings - Fork 75
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
Upgrade compiler v0.48 #2029
Upgrade compiler v0.48 #2029
Conversation
@@ -1246,23 +1246,23 @@ function emitUnion(context: SdkContext, type: Union): Record<string, any> { | |||
description: `Type of ${unionName}`, | |||
internal: true, | |||
type: "combined", | |||
types: sdkType.values.map((x) => getType(context, x.__raw)), | |||
types: sdkType.values.map((x) => getType(context, x.__raw!)), |
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.
@@ -1644,7 +1629,7 @@ export function emitCodeModel( | |||
simpleTypesMap.clear(); | |||
const allModels = getAllModels(dpgContext); |
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.
@iscai-msft @tadelesh After upgrading to the latest tcgc our inheritance test cases failed. I checked the getAllModels
interface would only return base model Pet
to us, in the old version it would return 3 models(1 base model + 2 derived models).
I believe getAllModels
should return both base and derived models, Could you check if there is a bug in tcgc?
autorest.typescript/packages/typespec-ts/test/modularUnit/modelsGenerator.spec.ts
Lines 771 to 805 in d94c5fd
it("should handle inheritance model", async () => { | |
const modelFile = await emitModularModelsFromTypeSpec(` | |
model Pet { | |
name: string; | |
weight?: float32; | |
} | |
model Cat extends Pet { | |
kind: "cat"; | |
meow: int32; | |
} | |
model Dog extends Pet { | |
kind: "dog"; | |
bark: string; | |
} | |
op read(): { @body body: Pet }; | |
`); | |
assert.ok(modelFile); | |
assertEqualContent( | |
modelFile?.getFullText()!, | |
` | |
export interface Pet { | |
name: string; | |
weight?: number; | |
} | |
export interface Cat extends Pet { | |
kind: "cat"; | |
meow: number; | |
} | |
export interface Dog extends Pet { | |
kind: "dog"; | |
bark: string; | |
}` | |
); |
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.
Since Pet
is not a discriminated type, Dog
and Cat
will not be used in read
, so getAllModel
will not return these two orphan models.
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.
Thanks for the info! I am checking the semantic with TypeSpec team: microsoft/typespec#2450.
export interface Test { | ||
color: "red" | "blue"; | ||
color: ColorType; |
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.
@iscai-msft @tadelesh Another failed test case is for union of string.
TypeSpec
model Test {
color: "red" | "blue";
}
Old generation after calling getSdkUnion
/** Type of ColorType */
export type ColorType = "red" | "blue";
export interface Test {
color: ColorType;
}
Latest generation with getSdkUnion
export interface Test {
color: "red";
}
After debugging I found the getSdkUnion
would only return the first enum member to us. Here is the relevant tcgc code.
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.
talked with the dpg crew about this, for sdk union types like this scenario, where the definition is `, we've decided to just emit it as a string. This is because there isn't information about the name of the union, and dotnet and java can't handle this. If they want an enum, they should define it as an enum in the tsp file, instead of us creating an enum from it
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.
That's interesting! I think emitting this as a string is not proper for TypeScript because we have native support for string union. And for TypeSpec type "red" | "blue"
, we may prefer to directly map to TypeScript type "red" | "blue"
like below. But currently we only have the first enum in underlying __raw, it makes us impossible to get all values.
On the other hand for other languages they emit as a string but they may have pontential requirement to touch underlying values, e.g using them in descriptions or knownable values etc.
export interface Test {
color: "red" | "blue";
}
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.
@joheredi Have you been involved in those discussions and are you agree with this ?
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.
IIRC we agreed to have it as a string in tcgc since 3/4 languages are just going to emit it as a string, which conflicts with what JS wants here. For JS though, I should be able to fix the __raw
to return the original union type and there should be enough information there. Down the line, we might decide that we can toggle this behavior with a flag, but since I think it's very JS specific, I think the best way right now is to have JS get the info they need themselves from the raw type
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.
Version 0.35.0-dev.5
is currently releasing and has this 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.
Thank you for the quick fix! Not sure we could have a stable patch version for this?
@@ -298,7 +298,7 @@ numeric, underscore or hyphen characters. | |||
|
|||
@summary("Create and start a new test run with the given name.") | |||
@doc("Create and start a new test run with the given name.") | |||
@pollingOperation(LoadTestRun.GetTestRun) | |||
// @pollingOperation(LoadTestRun.GetTestRun) |
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.
Prefer to disable this customized pollingOperation because it is not aligned with latest lro template.
https://github.com/Azure/typespec-azure/issues/3619#issuecomment-1729597022
}); | ||
|
||
// TODO: tgcg lib has a bug here: https://github.com/Azure/typespec-azure/issues/3623 | ||
it.skip("nullable string literal", async () => { |
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.
Skip this testing due to https://github.com/Azure/typespec-azure/issues/3623
@@ -100,13 +100,6 @@ export interface GetAzureBatchImageGenerationOperationStatusDefaultResponse | |||
GetAzureBatchImageGenerationOperationStatusDefaultHeaders; | |||
} | |||
|
|||
/** The final response for long-running getAzureBatchImageGenerationOperationStatus operation */ | |||
export interface GetAzureBatchImageGenerationOperationStatusLogicalResponse |
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.
The get polling operation won't be taken as lro in latest getLroMetadata
, so relevant logic response will be removed.
So I think this change is expected. Confirmed with .Net and Java they automatically filter out this polling method so it would not impact them.
@@ -171,15 +173,6 @@ function getDocStr(program: Program, target: Type): string { | |||
return getDoc(program, target) ?? ""; | |||
} | |||
|
|||
function isLro(_program: Program, operation: Operation): boolean { |
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.
No need to build modular lro helper because we can leverage existing one.
@@ -3097,6 +3096,50 @@ | |||
} | |||
} | |||
}, | |||
"TestRunCreateOrUpdate": { |
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.
The change is introduced by recent change: microsoft/typespec#2291
common/config/rush/pnpm-config.json
Outdated
"useWorkspaces": true, | ||
"strictPeerDependencies": false, | ||
"globalOverrides": { | ||
"@azure-tools/typespec-client-generator-core": "0.35.0-dev.5" |
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.
Pending tcgc release
Still one paging detail to confirm with Tim but may not block this pr: https://github.com/Azure/typespec-azure/pull/3382#discussion_r1332552725. |
@@ -672,6 +672,16 @@ | |||
"assignedRole" | |||
] | |||
}, | |||
"LedgerUserCreateOrUpdate": { |
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.
What is the root cause of the changing from LedgerUserUpdate to LedgerUserCreateOrUpdate ?
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.
see comment: #2029 (comment), so they change the visibility for patch method from update
to both update
and create
methods so relevant model name changed.
@@ -242,7 +231,7 @@ export function isUnexpected(response: CreateOrUpdateWidget200Response | CreateO | |||
export function isUnexpected(response: DeleteWidget202Response | DeleteWidgetLogicalResponse | DeleteWidgetDefaultResponse): response is DeleteWidgetDefaultResponse; | |||
|
|||
// @public (undocumented) | |||
export function isUnexpected(response: GetWidgetOperationStatus200Response | GetWidgetOperationStatusLogicalResponse | GetWidgetOperationStatusDefaultResponse): response is GetWidgetOperationStatusDefaultResponse; | |||
export function isUnexpected(response: GetWidgetOperationStatus200Response | GetWidgetOperationStatusDefaultResponse): response is GetWidgetOperationStatusDefaultResponse; |
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.
why missing the logic response here ?
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.
see comment: #2029 (comment)
@@ -38,13 +35,13 @@ export interface ListWidgetsOptions extends OperationOptions { | |||
|
|||
// @public (undocumented) | |||
export interface UpdateWidgetOptions extends OperationOptions { | |||
color?: ColorType; |
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.
Can we keep the colorType ? Is it from typepsec or codegen made it up ?
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.
The ColorType is made by TCGC, now they prefer to model it as string. See comments #2029 (comment).
Personally I prefer not to give a name for it. Because we don't know the name and it may conflict with customer's own.
@@ -10,11 +10,18 @@ export function getRLCClients(dpgContext: SdkContext): SdkClient[] { | |||
kind: "SdkClient", | |||
name: service.type.name + "Client", | |||
service: service.type, | |||
type: service.type | |||
type: service.type, | |||
arm: isArm(service.type) |
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.
why add this ? do we have any current different behaviors between ARM and non ARM in RLC ?
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.
In the latest tcgc the arm
becomes a required property and I copied the logic in tcgc to apply its value, but it is not used now.
@@projectedName(FooModel.x, "json", "xJson") | ||
@@projectedName(FooModel.x, "client", "NotToUseMeAsName"); // Should be ignored | ||
@@projectedName(FooModel.x, "javascript", "MadeForTS"); | ||
@@projectedName(FooModel.x, "json", "xJson"); |
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 it okay to add ; at the end of decorator ?
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.
format: sdkType.format, | ||
...extraInformation | ||
doc: "", | ||
apiVersions: [], |
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 the apiVersions info also getting removed in tcgc ? I think we need those when we are working on multi api version ?
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.
This is suggested by Isa, and it is only used to set the default value for simple model like string
so I think it's okay because we don't need to detect the version change for simple type.
Co-authored-by: Qiaoqiao Zhang <[email protected]>
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.
LGTM
* upgrade the tsp compiler version * Update the breakings * Fix to pass build errors * Update the ! * Fix the ut issue in rlc * Update the test cases * Update the test cases * update the lib to dev * Update the test cases * Update the spec * Update the lint * Disable the polling operation setting due to wrong def * Update .vscode/launch.json * Update .vscode/launch.json * Update the error spec * temp * temp * update the test cases * Update the test cases * update changes in openai * update the statement * Update the modular lro * Update the openapi.json * Update .vscode/launch.json * Update packages/typespec-ts/src/modular/buildCodeModel.ts Co-authored-by: Qiaoqiao Zhang <[email protected]> * update the name * Update the dependency to dev --------- Co-authored-by: Qiaoqiao Zhang <[email protected]>
Upgrade compiler v0.48