-
Notifications
You must be signed in to change notification settings - Fork 66
[TCGC] Fix: Allow custom encoding strings in SdkDateTimeTypeBase and SdkDurationType #3501
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
base: main
Are you sure you want to change the base?
Conversation
…rationType Co-authored-by: tadelesh <[email protected]>
tadelesh
left a 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.
@copilot Resolve the comments. Make sure pnpm run build and pnpm run test pass. Use pnpm -w change add to add changeset. Format the code with pnpm -w format:dir packages/typespec-client-generator-core.
Co-authored-by: tadelesh <[email protected]>
Changes applied in commit 4e757f1:
Note: Full build requires core submodule dependencies. The type changes are correct and tests for the modified functionality pass. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
.chronus/changes/fix-encode-custom-string-support-2025-11-11-07-28-39.md
Outdated
Show resolved
Hide resolved
tadelesh
left a 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.
Please hold merge before TypeSpec Nov. release.
commit: |
|
According to the decision here, TCGC decided to transparent pass the encode to language's emitter. Language's emitter could choose the behavior for unknown encode. |
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.
potentially break emitter due to the type of encode
The
@encodedecorator accepts arbitrary string values per TypeSpec compiler design, butSdkDateTimeTypeBase.encodeandSdkDurationType.encodewere typed asDateTimeKnownEncodingandDurationKnownEncoding, causing type mismatches for custom encodings.Changes
encodeproperty to use union typesDateTimeKnownEncoding | stringandDurationKnownEncoding | stringin bothSdkDateTimeTypeBaseandSdkDurationTypeinterfaces, providing better type safety while supporting custom encodingsas DateTimeKnownEncodingandas DurationKnownEncodingassertions in type constructionExample
The union type approach provides better TypeScript IntelliSense support for known encodings while still accepting any custom string value. The
isSdkDateTimeEncodings()type guard remains available for checking known encodings. All existing code continues to work unchanged.<issue_title>[Bug]: SdkDateTimeTypeBase.encode can be undefined when value isn't in DateTimeKnownEncoding</issue_title>
><issue_description>### Describe the bug
>
> The
@encodedecorator supports string values for arbitrary encodings.SdkDateTimeTypeBasedefinesencode: DateTimeKnownEncoding, and ifencodeisn't one of the well-known values, its value will beundefined.>
> ### Reproduction
>
> Compile tsp with a custom value for
@encodeon say autcDateTime.>
> ### Checklist
>
> - x] Follow our [Code of Conduct
> - x] Check that this issue is about the Azure libraries for typespec. For bug in the typespec language or core libraries file it in the [TypeSpec repo
> - [x] Check that there isn't already an issue that request the same bug to avoid creating a duplicate.
> - x] The provided reproduction is a [minimal reproducible example of the bug.</issue_description>
>
><agent_instructions>Make sure to run
pnpm -w formatandpnpm run testto get code formatted and tested.</agent_instructions>>
> ## Comments on the Issue (you are @copilot in this section)
>
>
><comment_new>@tadelesh
> When I implement the encode for datetime in TCGC, I supposed the encode for datetime and duration should be limited to known encoding. But it seems I'm wrong with that. The logic in compiler ensure specific encoding should only be applied to specific type. e.g., rfc3339 should only apply to datetime and string. Then it is reasonable that TCGC should support string encode.</comment_new>
>
>
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.