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

Allow enum flattening #202

Closed
wants to merge 12 commits into from
Closed

Allow enum flattening #202

wants to merge 12 commits into from

Conversation

escritorio-gustavo
Copy link
Contributor

This PR is aimed at allowing the use of #[flatten] in enum variants. It's not a very large change, but it does cause some code duplication, by redoing some of what format_variant does inside enum_def, so I'd like some ideas on how to mitigate that (maybe by changing format_variant to return variant_type?)

Fixes #96

@escritorio-gustavo escritorio-gustavo marked this pull request as draft January 23, 2024 13:38
@escritorio-gustavo
Copy link
Contributor Author

I'm converting this to a draft because I found a massive bug while testing further. My tests only had the enum itself in the struct, but adding other fields didn't work

@escritorio-gustavo
Copy link
Contributor Author

This feature requires changing all uses of interface to type in order to allow for type intersection (& operator)

@escritorio-gustavo
Copy link
Contributor Author

I have changed all the tests to account for this and added a few tests for the flattening of enums.

Although all the tests are passing, I'm still not 100% sure I haven't broken anything

@NyxCode
Copy link
Collaborator

NyxCode commented Jan 24, 2024

This feature requires changing all uses of interface to type in order to allow for type intersection (& operator)

I think this is fine. As far as I know, interfaces only additionally support declaration merging. I don't know for sure how common that pattern is, but I suspect not very often.

@escritorio-gustavo
Copy link
Contributor Author

I have only ever seen declaration merging when I used the next-auth library (their examples had it to override some cookies and sessions) as well as people wanting to add to Window

@NyxCode
Copy link
Collaborator

NyxCode commented Jan 24, 2024

Alright! So let's drop this for now, I think that's fine.
I've run a few sanity checks against serde-json, and the output of your flattened enums is consistant with it. Very nice!
Before this PR, I didn't even know it was possible to do that. Pretty awesome stuff.

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Jan 24, 2024

Alright! So let's drop this for now, I think that's fine. I've run a few sanity checks against serde-json, and the output of your flattened enums is consistant with it. Very nice! Before this PR, I didn't even know it was possible to do that. Pretty awesome stuff.

Thanks!
Btw. I think I got somewhat confused. Do you mean drop the PR or the declaration merging?

@escritorio-gustavo
Copy link
Contributor Author

I also think we need to figure out some better way to identify flattened enum fields, I did a whole lot of string manipulation there that I'm not sure will be able to handle everyone's use cases

@NyxCode
Copy link
Collaborator

NyxCode commented Jan 24, 2024

Alright! So let's drop this for now, I think that's fine. I've run a few sanity checks against serde-json, and the output of your flattened enums is consistant with it. Very nice! Before this PR, I didn't even know it was possible to do that. Pretty awesome stuff.

Thanks! Btw. I think I got somewhat confused. Do you mean drop the PR or the declaration merging?

:D I meant declaration merging! Would love to support flattened enums, I think this effort is great!

@escritorio-gustavo
Copy link
Contributor Author

Great, I might make a separate PR to change interfaces to types and simplify this one when it's ready

@NyxCode
Copy link
Collaborator

NyxCode commented Jan 24, 2024

Awesome, take your time!

I also think we need to figure out some better way to identify flattened enum fields, I did a whole lot of string manipulation there that I'm not sure will be able to handle everyone's use cases

I 100% agree! Once you think this is ready, feel free to convert it back to a PR again, then I'll take an other look.
Let me know as well if you get stuck, I'd definetely be happy to help!

@escritorio-gustavo
Copy link
Contributor Author

I think I figured it out. I created a new local branch to start from scratch and managed to get something working by converting all uses of flatten into type intersections.

This makes for slightly more redundant TS code but it seems to work. Example with the flatten test:

type Original = { b: { a: number, b: number, c: number, }, d: number, }

type New = { b: { c: number, } & { a: number, b: number, }, d: number, }
//              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ More verbose way to express the same type

This pretty much eliminated the string manipulation mess I was previously doing

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Jan 25, 2024

@NyxCode should I close this PR and submit the new branch as a new one or transfer the code over here?

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Jan 25, 2024

Btw the extra intersection can be solved by adding .replace(" } & { ", " "), which should generate the following:

type Original = { b: { a: number, b: number, c: number, }, d: number, }

type ExplicitIntersection = { b: { c: number, } & { a: number, b: number, }, d: number, }

type ImplicitIntersection = { b: { c: number, a: number, b: number, }, d: number, }
//                                 ^^^^^^^^^ Less verbose, but the fields will still be out of order
//                                           (non-flattened fields always come before flattened ones)

This is possible because an enum will never be directly adjacent to a struct, as it will always be in parenthesis
{ /*...fields*/ } & ({ /*...fields*/ } | /*...variants*/)

@NyxCode
Copy link
Collaborator

NyxCode commented Jan 25, 2024

I think I figured it out. I created a new local branch to start from scratch and managed to get something working by converting all uses of flatten into type intersections.

This makes for slightly more redundant TS code but it seems to work. Example with the flatten test:

type Original = { b: { a: number, b: number, c: number, }, d: number, }

type New = { b: { c: number, } & { a: number, b: number, }, d: number, }
//              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ More verbose way to express the same type

This pretty much eliminated the string manipulation mess I was previously doing

Awesome! Yeah, I don't see any issues with this whatsoever. The generated TS might be a bit more unintuitive that way, but I don't think anyone cares about that.

@NyxCode
Copy link
Collaborator

NyxCode commented Jan 25, 2024

@NyxCode should I close this PR and submit the new branch as a new one or transfer the code over here?

Whatever you think is best! Generally, I think splitting a change like this into small PRs (doing required refactorings first, then implementing the feature) is a great idea.

@escritorio-gustavo
Copy link
Contributor Author

Great, in that case I'll close this PR and open a new one with the new branch

@escritorio-gustavo
Copy link
Contributor Author

The generated TS might be a bit more unintuitive that way, but I don't think anyone cares about that.

I managed to change that in the latest commit

@escritorio-gustavo escritorio-gustavo deleted the add_enum_flattening branch January 25, 2024 16:22
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.

Unable to generate flattened enum
2 participants