-
Notifications
You must be signed in to change notification settings - Fork 183
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
Simplify usage of attributes. #574
Merged
Merged
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
This commit adds support for strings, numbers, and booleans to be implicitly typed in attribute maps, reducing the redundancy of needing to specify their types. I also quietly adjusted one of the tests to use a more stable class/property pair. Since SourceAssetId is locked to Roblox, it could potentially disappear at any time.
LPGhatguy
requested changes
Jul 8, 2022
MaximumADHD
changed the title
Support implicit values for primitive attributes
Support unambiguous values in attributes
Jul 8, 2022
MaximumADHD
changed the title
Support unambiguous values in attributes
Support writing unambiguous primitives directly in attributes.
Jul 8, 2022
LPGhatguy
added
the
status: needs review
This work is mostly done, but just needs work to integrate and merge it.
label
Jul 9, 2022
Attributes can be defined directly on instances, with support for unambiguous types.
MaximumADHD
changed the title
Support writing unambiguous primitives directly in attributes.
Simplify usage of attributes.
Jul 12, 2022
LPGhatguy
approved these changes
Aug 3, 2022
Dekkonot
pushed a commit
to UpliftGames/rojo
that referenced
this pull request
Jan 11, 2024
* Support implicit values for primitive attributes This commit adds support for strings, numbers, and booleans to be implicitly typed in attribute maps, reducing the redundancy of needing to specify their types. I also quietly adjusted one of the tests to use a more stable class/property pair. Since SourceAssetId is locked to Roblox, it could potentially disappear at any time. * Apply formatting. * Address feedback * Backwards compatible format usage. * Axe UnresolvedValueMap in favor of $attributes Attributes can be defined directly on instances, with support for unambiguous types. * Adjust test. * to_string() -> into() * Made attribute test more concise. * small cleanup * Update src/resolution.rs * Update src/resolution.rs * Update src/resolution.rs * Update src/resolution.rs Co-authored-by: Lucien Greathouse <me@lpghatguy.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
status: needs review
This work is mostly done, but just needs work to integrate and merge it.
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.
Summary
This commit deprecates using the
Attributes
property in$properties
, in favor of a direct$attributes
field in the JSON instances (orattributes
in*.model/meta.json
files). Unambiguous value types likestring
,boolean
, andnumber
can be written as fields of$attributes
directly.The Problem
In trying to clean up the existing structure for writing attributes (i.e. adding unambiguous value types), it became apparent the following case would cause problems with how rojo handles explicit types:
Rojo interprets the value of this object's attributes to be an explicit string
"Test"
, rather than a single attribute named"String"
with"Test"
as its value. Working around this requires the value of test to be encapsulated as so:The solution is to explicitly make
$attributes
a separate field:This not only looks cleaner, but it also clears up any ambiguity since
$attributes
is explicitlyHashMap<String, Variant>
.Compatibility
This change is forward compatible with the existing use of the
"Attributes"
property.It just requires noisy explicit typing to work as expected.