[Bug]: SdkFloatKindsEnum contains decimal types #1106
Comments
timotheeguerin
added
the
lib:tcgc
tadelesh
added a commit
that referenced
this issue
Jul 23, 2024
… `SdkDateTimeType` and `SdkDurationType` (#1015) Fixes #1010 Fixes #997 Fixes #1106 # Changes and migration guides ## Changes 1. remove the following types from `SdkBuiltInType`: - `uuid` (azure type) - `ipV4Address` (azure type) - `ipV6Address` (azure type) - `eTag` (azure type) - `armId` (azure type) - `azureLocation` (azure type) - `password` (removed) - `guid` (removed) - `uri` (removed) - `ipAddress` (removed) 2. `@format` can no longer change a string type to above azure types (`uuid`, `eTag`, etc). 3. add `name`, `crossLanguageDefinitionId` and `baseType` to `SdkBuiltInType`, `SdkDateTimeType` and `SdkDurationType`. 4. now scalars defined using `scalar` keyword will be parsed into either `SdkBuiltInType`, `SdkDateTimeType` or `SdkDurationType` depending on the base type, with its `name` and `crossLanguageDefinitionId`. `@encode` will be added to the scalar type, and will not propagate to its base type. 5. `isSdkFloatKind` now returns `false` for `decimal` and `decimal128`. ## Migration guides for emitters The major breaking change introduced by this change is the removal of azure related kinds for `SdkBuiltInType`. Therefore for code such as: ```typescript if (type.kind === "azureLocation") { // do something for azure-location } ``` should be changed to the following if the emitter is doing something special for azure-location: ```typescript if (type.kind === "string" && type.crossLanguageDefinitionId === "Azure.Core.azureLocation") { // do something for azure-location } ``` or just change it to this: ```typescript if (type.kind === "string") { // treat azure-location as a string } ``` For those removed kinds, they no longer exist therefore it should be safe just remove related code snippets. # Summary This PR includes the following changes: 1. remove the following types from `SdkBuiltInType`: - `uuid` (azure type) - `ipV4Address` (azure type) - `ipV6Address` (azure type) - `eTag` (azure type) - `armId` (azure type) - `azureLocation` (azure type) - `password` (removed) - `guid` (removed) - `uri` (removed) - `ipAddress` (removed) 2. `@format` can no longer change a string type to above azure types (`uuid`, `eTag`, etc). 3. add `name`, `crossLanguageDefinitionId` and `baseType` to `SdkBuiltInType`, `SdkDatetimeType` and `SdkDurationType`. 4. now scalars defined using `scalar` keyword will be parsed into either `SdkBuiltInType`, `SdkDatetimeType` or `SdkDurationType` depending on the base type, with its `name` and `crossLanguageDefinitionId`. `@encode` will be added to the scalar type, and will not propagate to its base type. 5. `isSdkFloatKind` now returns `false` for `decimal` and `decimal128`. # Reasons and motivations TCGC should be "linguistic neutrality" which does not favor or disfavor any language and only represent the structures defined in a tsp file/files. Also I think it should be neutral for 3rd party extensions, and azure should be regarded as our first 3rd party extension. Therefore we need a way to be able to handle those azure types without making them special in TCGC's code. The solution that this PR is applying is similar to the solution we have for models in azure core template. Instead of creating a special kind for models in azure core template (such as the `ResponseError`, `TrackedResource`, `Resource` or `ExtendedLocation`), we just parse them as regular models with their own name and tsp namespace, so that the downstream genreators could know where these types are defined, and if they want, they could do a filter, to handle those special types. Same thing could apply to scalars, for a scalar defined in azure core template: ```typespec namespace Azure.Core; scalar azureLocation extends string; ``` for those would care this, this stands for an "azure location", for those would not care, this is just a string. Therefore this PR introduces name and `crossLanguageDefinitionId` to `SdkBuiltInType` just like we have in models, this scalar now will be parsed as: ``` { kind: "string", name: "azureLocation", tspNamespace: "Azure.Core", baseType: { kind: "string", name: "string", tspNamespace: "TypeSpec" } } ``` The `crossLanguageDefinitionId` is important for downstream generators to know whether this scalar is defined. `baseType` here is also important and we must have this, because we do not want to see if users write scalars extending some of the scalars from azure core template, and it ends up with a string. For example, based on the fact that we have the above `azureLocation` defined in our azure core template, and one could write: ```typespec scalar myLocation extends azureLocation; model Foo { bar: myLocation; } ``` in this case, I think the generator should generate the `bar` property of `Foo` model as `AzureLocation` type. But if we do not have the `baseType`, we only have this: ```typespec { kind: "string", name: "myLocation", tspNamespace: "My.Service" } ``` which has nothing with `azureLocation` which is not acceptable. --------- Co-authored-by: tadelesh <tadelesh.shi@live.cn>
|
Resolved by #1015 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
Decimal types are Fixed Points, not Floating Points, therefore they shouldn't be in
SdkFloatKindsEnum- either name the enum "SdkFractionalKindsEnum", or split it into "SdkFloatingPointKindsEnum" and "SdkFixedPointKindsEnum" (or do both -SdkFractionalKindsEnum = SdkFloatingPointKindsEnum | SdkFixedPointKindsEnum).Reproduction
See
typespec-azure/packages/typespec-client-generator-core/src/interfaces.ts
Lines 137 to 143 in 54567f7
Checklist
The text was updated successfully, but these errors were encountered: