-
Notifications
You must be signed in to change notification settings - Fork 49
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
Implement enum attributes #470
Implement enum attributes #470
Conversation
Hopefully less confusing this way
Add EnumItem to generated values
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.
This is potentially annoying for upstream Rojo users (they'll have to use EnumItem
instead of Enum
for attributes) but it's probably fine?
Do want to implement serialization for this type when it's used as a property so that consumers don't have to be careful?
Yeah, it's a bit annoying, but this isn't particularly common use case, the shape of the data is different, and Rojo users only have to write the explicit syntax for enum properties very rarely, so confusion between EnumItem and Enum should be kept to a minimum |
Realized immediately after I hit approve that this needs a changelog entry... 😅 |
Is just the rbx_dom_weak changelog okay? |
...I was more thinking an rbx_types changelog entry. |
Oh god, I'm losing the plot! |
This PR closes #383 by by adding a new tagged enum variant,
EnumItem
, that contains the name of theEnumItem
as well as its value, and implementing the attributes reader/writer against the specification at rojo.dom.space.Additionally, it adds a codec to rbx_dom_lua to handle the new
EnumItem
variant.It also updates some docs, since the link that's currently there doesn't work anymore.
I went back and forth on what the fields should be named in the serde representation, and settled on
type
andvalue
. This more or less mirrorsEnumItem.EnumType
andEnumItem.Value
on Roblox - but maybetype
should beenumType
instead?