-
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
support encode for modular #2006
support encode for modular #2006
Conversation
name: p["name"], | ||
values: p["values"], | ||
})), | ||
}, |
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.
Could you point to me where is the fix code?
@@ -63,8 +63,7 @@ export function _publishCloudEventSend( | |||
? uint8ArrayToString(event["dataBase64"], "base64") | |||
: undefined, | |||
type: event["type"], | |||
time: | |||
event["time"] !== undefined ? new Date(event["time"]) : undefined, | |||
time: event["time"]?.toISOString(), |
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.
I notice the typespec type here is utcDateTime
but we generate to toISOString
Not sure it is possible to add some UTs to cover typespec type to modular type:
/**
* A date on a calendar without a time zone, e.g. "April 10th"
*/
scalar plainDate;
/**
* A time on a clock without a time zone, e.g. "3:00 am"
*/
scalar plainTime;
/**
* An instant in coordinated universal time (UTC)"
*/
scalar utcDateTime;
/**
* A date and time in a particular time zone, e.g. "April 10th at 3:00am in PST"
*/
scalar offsetDateTime;
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.
I suspect typespec compiler does not provide any format info to these types, but let me check.
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.
typescript date.toUTCString() is "Fri, 08 Sep 2023 08:50:03 GMT" and date.toISOString() is "2023-09-08T08:42:49.439Z"
therefore use toISOString for utcDateTime makes more sense. and toUTCString for offsetDateTime makes more sense to me.
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.
Yeah, it makes sense to me also. Just checking with Tim here microsoft/typespec#1899 (comment)
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 seems both of them share the same default value.
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.
return `"${param.restApiName}": options?.${param.clientName} !== undefined ? ${collectionInfo}(options?.${param.clientName}${additionalParam}): undefined`; | ||
if (!param.optional) { | ||
return `"${param.restApiName}": ${collectionInfo}(${serializeRequestValue( | ||
param.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.
I think the change makes sense for me.
param.type.type === "datetime" | ||
? "headerDefault" | ||
: param.format | ||
)}`; |
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 should be here fixing the options.filters
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 and just left small comments for checking and one ut.
add support for encode for datetime, duration, bytes in Modular.
for datetime, Modular layer will always be a Date type, but RLC layer will be different.
serialize: someDate.toUTCString()
deserialize: Date(someUTCString)
serialize: someDate.getTime()
deserialize: Date(someUnixTimestamp)
serialize: someDate.toISOString()
deserialize: Date(someISOString)
for duration
for bytes:
serialize: uint8ArrayToString(value, format)
deserialize: stringToUint8Array(value, format)
fixes #2007