rojo-rbx / rbx-dom

Roblox DOM and (de)serialization implementation in Rust
MIT License
105 stars 42 forks source link

Enum attributes #396

Open krakow10 opened 4 months ago

krakow10 commented 4 months ago

For #383

This is my guess as to how to implement this, assuming there is some sort of string in the serialized attributes. It builds but it does not work. Some sort of reflection database error that I don't know what it means.

thread 'main' panicked at /home/quat/git/rbx-dom/rbx_reflection_database/src/lib.rs:7:76:
could not decode reflection database because: invalid type: integer `0`, expected struct Enum
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
krakow10 commented 4 months ago
println!("heyyy name={} value={}", enum_name, enum_value);

image I guessed right :sunglasses: still don't know how to work the reflection database

Dekkonot commented 4 months ago

It builds but it does not work. Some sort of reflection database error that I don't know what it means.

This is because you're changing the (de)serialization of Enum! As a result, the reflection database can't be deserialized by the new version you're making.

Fixing this is an involved processs because we bootstrap the database. As a result I don't really recommend you try to fix it because it's potentially error prone and involves a bit of juggling with reflection databases.

PS: We have the enum attributes documented in the spec file we maintain. You didn't have to guess. 😄

krakow10 commented 4 months ago

What about a separate explicit enum type, like this?

krakow10 commented 4 months ago

This branch is now set up so that it suppresses the enum attribute errors and provides weakly typed enum functionality. The explicit enum is moved to a different branch, never to be heard from again.

Dekkonot commented 4 months ago

This implementation would fail a round-trip test because Enums no longer contain information on their type.

krakow10 commented 4 months ago

Well yes... but this is still useful as an error-suppressing patch while the full functionality isn't available

Dekkonot commented 4 months ago

I will put aside time next week to fix this properly since it's showing up in the wild now, but I'm not willing to merge a feature that doesn't roundtrip. We won't be making a new release without the full feature anyway, so it being temporary fix is moot.

If it's desperate, I'd suggest using a custom fork of whatever tool you're using for the next little while.

krakow10 commented 4 months ago

I don't think it should be merged either which is why I have it as a draft, sorry for the misunderstanding. I'm not trying to get this incomplete feature merged, just a publicly available error fixer