-
Notifications
You must be signed in to change notification settings - Fork 165
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
Adopt scalar type changes from TCGC #4883
Conversation
internal const string ContentTypeId = "Azure.Core.contentType"; | ||
internal const string ResourceTypeId = "Azure.Core.resourceType"; | ||
|
||
internal const string ObjectId = "Obsolete.object"; |
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 Obsolete.object
?
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 comment shares the same reply here.
Previously we are using some special InputPrimitiveTypeKind
to handle some legacy types from swagger input.
In this PR since we need to keep the InputPrimitiveTypeKind
identical as SdkBuiltInTypeKinds
from TCGC, we no longer could add more kinds into the type (or in other words, we should not), therefore here we just construct a new scalar type here internally with a special crossLanguageDefinitionId
, so that the TypeFactory
could know which type to return.
internal const string ObjectId = "Obsolete.object"; | ||
internal const string RequestMethodId = "Azure.Core.requestMethod"; | ||
|
||
internal const string IPAddressId = "Temp.ipAddress"; |
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 Temp.ipAddress
?
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 #4883 (comment)
For ipAddress, maybe we could change this to share with ipv4Address
or ipv6Address
.
These are not decided yet.
}; | ||
} | ||
|
||
private static readonly IReadOnlyDictionary<string, CSharpType> _knownPrimitiveTypes = new Dictionary<string, CSharpType> |
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 criteria of _knownPrimitiveTypes
and InputPrimitiveTypeKind
?
Or to say, how to define a type kind should go to _knownPrimitiveTypes
or InputPrimitiveTypeKind
?
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 key here is the crossLanguageDefinitionId
.
And based on the principal that we should keep minimum logic in our emitter only - just taking everything TCGC gives us, the InputPrimitiveTypeKind
should have exactly the same values as SdkBuiltInTypeKinds
. And in my TCGC part, this SdkBuiltInTypeKinds
is defined like this:
export type SdkBuiltInKinds = Exclude<IntrinsicScalarName, SdkBuiltInKindsExcludes> | "any";
type SdkBuiltInKindsExcludes = "utcDateTime" | "offsetDateTime" | "duration";
Therefore, there should never be a situation that we should add a new type kind into this inputPrimitiveTypeKind, unless typespec is updated and decide to add a new kind into IntrinsicScalarName
which means a new std type is added into typespec.
Encode: DurationKnownEncoding; | ||
WireType: InputPrimitiveType; | ||
BaseType?: InputDurationType; |
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.
we are not handling the newly added BaseType
in generator?
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.
We do not take it right now because we do not have a case yet.
We have it here, because the input types should be identical to the type system provided by TCGC
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.
For more details, please refer the description of this PR: Azure/typespec-azure#1015
Adopt latest changes from TCGC.