Perform property migrations in rbx_xml serializer #340
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| <Item class="Part" referent="4"> | ||
| <Properties> | ||
| <string name="Name">Part</string> | ||
| <Color3uint8 name="Color">11829503</Color3uint8> |
There was a problem hiding this comment.
Maybe the new property for MigrationOperation::BrickColorToColor should really be Color3uint8? 🤔 It'll still work regardless though...
There was a problem hiding this comment.
I suspect yes, it should be. With the way Roblox handles reflection, it turns out to be fine, but we should probably fix that for correctness's sake.
kennethloeffler
approved these changes
Aug 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A companion to #339 and related to #335 as well.
As hoped for in 339, this was in fact much easier. We already have a mechanism for replacing names and values during serialization in rbx_xml, so it wasn't hard to add migrations to that process.
Unlike in the rbx_binary implementation, there's no compatibility concern if a migration fails. I've noted that in the source, but for posterity, if one fails and we allow a property to not be migrated, it'll still load just fine in Studio so there's not any real concern.