Skip to content
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

Add "constant" to DurationKnownEncoding #5672

Closed
wants to merge 1 commit into from

Conversation

pshao25
Copy link
Contributor

@pshao25 pshao25 commented Jan 21, 2025

It starts from Azure/typespec-azure#1901. To represent "x-ms-format": "duration-constant", we are expecting something like

model Test {
  @encode(DurationKnownEncoding.constant)
  input: duration;
}

What's more, we might need to change in TypeSpec-Azure to emit "x-ms-format": "duration-constant" in the swagger.

@microsoft-github-policy-service microsoft-github-policy-service bot added the compiler:core Issues for @typespec/compiler label Jan 21, 2025
@azure-sdk
Copy link
Collaborator

❌ There is undocummented changes. Run chronus add to add a changeset or click here.

The following packages have changes but are not documented.

  • @typespec/compiler
Show changes

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs 🛝 VSCode Extension

/**
* Encode for duration-constant
*/
constant: "constant",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we get more detail about what this format is, if its not something defined by an rfc it shouldn't be added here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the description of this issue, Azure/typespec-azure#1901, we are converting Compute swagger to TypeSpec. They have defined "x-ms-format": "duration-constant", like below:

"maximumDuration": {
  "type": "string",
  "x-ms-format": "duration-constant"
},

which impacts the deserialization, meaning SDK does different serialization in the payload for this specific property.

In the description of that issue, I have tried two ways to represent it but failed. Therefore, we propose to add a "constant", so that we can do

model Test {
  @encode(DurationKnownEncoding.constant)
  input: duration;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've no idea whether it is RFC, but we need a solution if we don't add "constant".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

X-Ms-format though wasn't an autorest core thing though?

What is the format of serialization where does it come from?

This seems like quite an edge case to consider this level of support

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. Then how about we enable

model Test {
  @format("duration-constant")
  input: duration;
}

to make it pass compilation.

Copy link
Member

@ArcturusZhang ArcturusZhang Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not we just simply use

model Test {
	@encode("constant") // or duration-constant
	input: duration;
}

encode decorator could accept any string as values, and if when you do this, TCGC is not able to handle them properly, this is a bug in TCGC.
We do not really have to introduce such a value into this enum.

And FYI @format might work fine on swaggers, but the SDK generators now ignore format therefore it will not work for SDKs.

On the other hand, if you could find this format in RFC, we could have it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the description of that issue with trial 3. We need an "x-ms-format": "duration-constant", not an "format": "duration-constant".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, @ArcturusZhang is right, encore doesn't require the value to be defined there is is open. You can support whatever additional custom encoding you want without the standard library defining it.

Format is incorrect as you saw for 2 reason it's for format not x-ms-format and it's about giving a known pattern to a string so will never work with anything else

@pshao25 pshao25 closed this Jan 23, 2025
@pshao25 pshao25 deleted the duration1901 branch January 23, 2025 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:core Issues for @typespec/compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants