-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
C-Style Enums #36
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
C-Style Enums #36
Conversation
Awesome, thanks! Would it be possible to add some tests for this as well? Additionally, for the JS bindings side of things, perhaps the names of the variants could be exported as well? |
I will add tests for sure! For the variant exports did you mean the following? export const MyEnum = {Red: 1, Blue: 2};
export const Red = "Red";
export const Blue= "Blue"; |
IMO for signature parity, the translated enum should be: type MyEnum =
| { A: any }
| { B: { '0': number } }
| { C: { '0': number, '1': number } }
| { D: { x: number, y: number } }
; |
@dbkaplun I guess you meant to comment on the other issue? This PR covers only C-style enums. |
@rylev As far as I understand from code, you're currently generating entries as constant object? Maybe it would make sense to use TypeScript's native enum for clarity like following? enum Direction {
Up,
Down,
Left,
Right,
} |
@RReverser the generated code is pure JavaScript so it's not possible to use TypeScript enums. Or were you referring to when we generate the appropriate stuff for the .d.ts file? |
@rylev Oh, sorry, I misunderstood that it's pure JS generation. I guess "Generates Typescript for now instead of JS (although that may come later)" from README should be excluded now? |
@RReverser hmm interesting. I didn't notice that in the README. The output is definitely just JavaScript (i.e. no type annotations). I wonder what @alexcrichton meant by that... |
pass(&name) | ||
} | ||
shared::TYPE_NUMBER => { | ||
shared::TYPE_ENUM | shared::TYPE_NUMBER => { |
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.
Is it not possible to expose actual enum name instead of generic number
?
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 is. It requires a bit of a refactoring of how things are handled though. Specifically we lose all information about enums inside of functions right now, and we'll have to refactor how we handle "custom types" to make it work. I would prefer to save it for the next PR where I'll handle non-C-style enums. The changes necessary for that will sure this up.
@alexcrichton This is ready to go. I will start looking into supporting enums with associated data. This might be a good time to look into refactoring how custom types are handled. Right now all "custom types" are assumed to be represented as classes in JS. This breaks down with enums. Having a more flexible definition of custom types will help us generate better JS. If you have any time to chat about this, let me know. |
This looks great to me, thanks so much @rylev! Also yeah the README may be a bit outdated, this used to generate typescript by default but has since switched to js. I'll update that! |
@rylev Did further work on Enum support happen after this PR? |
Fixes #31. This is a very basic version of C-Style enum support.
Some limitations are:
All these limitations should be fairly easy to overcome, but I wanted to get this reviewed sooner rather than later since I'm new to the codebase. If this is merged, I can work on fixing the limitations (especially the first two).
I wasn't sure if this was the best way to structure the code, so feedback is very welcome :-D