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

Fix: enum serialization #5309

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Kindest13
Copy link
Contributor

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!
In the issue you were mentioning that you're having issue implementing a unit test.
Can you please also include the draft unit test in this pull request?

@Kindest13
Copy link
Contributor Author

Thanks for the contribution! In the issue you were mentioning that you're having issue implementing a unit test. Can you please also include the draft unit test in this pull request?

Hey @baywet
I was debugging existing test and wanted to update its assertion (to remove spread operator)
line 1104 WritesConstructorWithEnumValueAsync in CodeFunctionWriterTests.cs
But seems like we pass enum but it's not recognized as one

@baywet
Copy link
Member

baywet commented Sep 4, 2024

@Kindest13 I've just pushed an example unit test. I think the changes are getting us to the right direction but are not enough yet.
There are effectively 3 different enum cases:

  • single value => we already have what we need in place end to end.
  • flaggable => I think this is why we originally introduced the spread operator. What I don't know is how anybody would set those properties/if we need to correct the generation for those kinds of properties. I think we'd need to default to an array which will be serialized as CSV since there's no flag or enum set notion in TypeScript
  • collection of enum values => here we're most likely missing a method in the kiota-typescript SerializationWriter and ParseNode interfaces. If you want you can go ahead and start a pull request over there as well.

Regardless, all methods need to ALSO accept the serialization object so we can map enum members to their serialization representations, which is not implemented today.

Let us know if you have any additional comments or questions.

I'd also like @andrueastman opinion on this one.

@andrueastman
Copy link
Member

flaggable => I think this is why we originally introduced the spread operator. What I don't know is how anybody would set those properties/if we need to correct the generation for those kinds of properties. I think we'd need to default to an array which will be serialized as CSV since there's no flag or enum set notion in TypeScript

I think this also what we were trying to solve with the x-ms-enums-flags extension with the idea of having also specify the serialization format.
This way a language like TS could specify the enum in both case 2 and 3 as a collection of enum values then the serializer would do the job knowing what to write (csv string or collection of strings) using the type specified.

I agree with making the property default as a collection for now. That requires us to pass extra information to the serializer to differentiate the case 2 and 3. But that is solved with the separate methods for now (once we add the method for collection of enum values).

Similar to GO we could also generate the string conversion function for the enum but that would lead to a bigger footprint in generation.

@baywet
Copy link
Member

baywet commented Sep 10, 2024

Thank you for the additional information @andrueastman
The extension itself is for future work, and likely a source breaking change to coordinate across all languages.

If we focused on "this version of the generation across all languages" I believe we could "align" TypeScript with the changes I've described in my previous reply. Anything I might have missed? Or issues? In your opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong serializer used if array of string uses enum
3 participants