Implement property migration #283
Conversation
Don't need these anymore!
Giving myself an excuse for a future variable rename - I think "type_name" is pretty poor because it's overloaded by other parts of rbx_binary and also I'm pretty used to calling this concept "class name!"
This allows the pattern matching in rbx_xml and rbx_binary to be a little simpler and renames what was formerly "Property" to "MigratesTo," which I think is clearer
Dunno. Lemme know!
kennethloeffler
force-pushed
the
property-migration-pr-fix
branch
from
b293ddb to
bd42512
Compare
| // TODO: Do we need an additional fix here? | ||
| let canonical_name = &descriptors.canonical.name; | ||
| let canonical_type = match &descriptors.canonical.data_type { | ||
| DataType::Value(ty) => *ty, | ||
| DataType::Enum(_) => VariantType::Enum, | ||
| _ => { | ||
| // TODO: Configurable handling of unknown types? | ||
| return None; | ||
| } | ||
| }; |
There was a problem hiding this comment.
I realize these are copied verbatim from the old source but it does make me wonder what was actually meant by the first one.
With regards to the second one, we don't really need to worry about it because I don't foresee a world where Roblox adds something that's not a data type or an enum that also serializes.
There was a problem hiding this comment.
I realize these are copied verbatim from the old source but it does make me wonder what was actually meant by the first one.
With regards to the second one, we don't really need to worry about it because I don't foresee a world where Roblox adds something that's not a data type or an enum that also serializes.
These comments were added by #230.
The first comment might be talking about other possible ways rbx_binary can serialize properties that aren't supposed to serialize? But I'm not sure what other cases could lead to this kind of problem 🤷
The fall through is hit on an unknown property type, and the second comment is referring to doing something smarter in that case. I think we should keep this comment.
| "String": "{7EFC8201-25F5-42C7-8DDB-DD7BD2D32FE7}" | ||
| "String": "{7604441B-B379-4EE1-984B-AE157BC63C77}" |
There was a problem hiding this comment.
We should probably do something about ScriptGuid going forward because it seems like it isn't stable between versions... Which is probably expected behavior given it's a GUID.
It doesn't need to and shouldn't happen in this PR but it's something to think about because I'm not sure how to actually solve a property that we shouldn't store a default for like this.
There was a problem hiding this comment.
Agreed that it doesn't make sense to have a default ScriptGuid - we ought to catalogue any properties for which defaults aren't sensible and figure out a good strategy to deal with them
Co-authored-by: Micah <[email protected]>
Co-authored-by: Micah <[email protected]>
Co-authored-by: Micah <[email protected]>
Based on PR rojo-rbx#268 Co-authored-by: Shae A <[email protected]>
This PR is based off of #268 - all of the description there also applies here. The only thing that really needed to be done was supporting Migrate patches in rbx_reflector.
I've also slightly refactored the whole canonical property dance in the rbx_binary deserializer state, hopefully it's a bit easier to follow.
TODO: