Serialize UniqueId properties + support setting default property value in patches #437
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
|
Yeah I'm not super jazzed either, but it's what we gotta do for now because the other option is committing to a broader change. We should probably prioritize it, but I don't think it's worth crying over right now. |
kennethloeffler
changed the title
|
Last thing: I've edited the PR's title to mention that it's making UniqueId props serialize again, since we've had problems with UniqueId in the past. If this title isn't quite to your liking feel free to change it |
kennethloeffler
approved these changes
Aug 16, 2024
Dekkonot
changed the title
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.
As discussed in #435 and previously in #418, this allows us to specify the default value for properties using patches in rbx-reflector.
In addition to that, this PR also sets a default for
Archivableand also reverts #327 and overrides their defaults instead. Since several of these properties are onInstance, this results in a very large change to the reflection database.We can and should in the future make it so defaults are stored in a method that respects inheritance and encourage people to use
ReflectionDatabase::find_default_propertyto find defaults. The reason I'm not doing it at this moment is that historically people haven't had to care about inheritance to get defaults. Though it's not a breaking change, I feel like we should give some warning if we change it so they have to, especially for critical things likeArchivable.