Skip to content
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 15 commits into from
Aug 4, 2022
Merged

Conversation

MaximumADHD
Copy link
Contributor

@MaximumADHD MaximumADHD commented Jul 8, 2022

Summary

This commit deprecates using the Attributes property in $properties, in favor of a direct $attributes field in the JSON instances (or attributes in *.model/meta.json files). Unambiguous value types like string, boolean, and number 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:

{
  "$properties": {
    "Attributes": {
      "String": "Test"
    }
  }
}

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:

{
  "$properties": {
    "Attributes": {
      "String": {
        "String": "Test"
      }
    }
  }
}

The solution is to explicitly make $attributes a separate field:

{
  "$attributes": {
    "String": "Test"
  }
}

This not only looks cleaner, but it also clears up any ambiguity since $attributes is explicitly HashMap<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.

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.
src/resolution.rs Outdated Show resolved Hide resolved
src/resolution.rs Outdated Show resolved Hide resolved
src/resolution.rs Outdated Show resolved Hide resolved
@MaximumADHD MaximumADHD changed the title Support implicit values for primitive attributes Support unambiguous values in attributes Jul 8, 2022
@MaximumADHD MaximumADHD changed the title Support unambiguous values in attributes Support writing unambiguous primitives directly in attributes. Jul 8, 2022
@LPGhatguy 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 MaximumADHD changed the title Support writing unambiguous primitives directly in attributes. Simplify usage of attributes. Jul 12, 2022
src/resolution.rs Outdated Show resolved Hide resolved
src/resolution.rs Outdated Show resolved Hide resolved
src/resolution.rs Outdated Show resolved Hide resolved
src/resolution.rs Outdated Show resolved Hide resolved
@LPGhatguy LPGhatguy merged commit d196c50 into rojo-rbx:master Aug 4, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants