feat!: turning List<Property> into CustomProperties #55

kurtome · 2022-09-15T02:51:49Z

Description

See #54

Checklist

The title of my PR starts with a [Conventional Commit] prefix (fix:, feat:, docs: etc).
I have read the [Contributor Guide] and followed the process outlined for submitting PRs.
I have updated/added tests for ALL new/updated/fixed functionality.
I have updated/added relevant documentation in docs and added dartdoc comments with ///.
I have updated/added relevant examples in examples.

Breaking Change

This is a breaking change, but I'm intentionally not using feat! because we don't want to bump the major version.

Yes, this is a breaking change.
No, this is not a breaking change.

Migration instructions

Use properties.named<TypeProperty>('prop_name').value where applicable. Also, CustomProperties implements Iterable<Property>.

Related Issues

Fixes #54

jtmcdole

👍🏻

jtmcdole · 2022-09-15T03:14:42Z

packages/tiled/lib/src/common/property.dart

+
+    return properties.groupFoldBy((prop) => prop.name, (previous, element) {
+      if (previous != null) {
+        throw ArgumentError("Can't have two properties with the same name.");


This seems like a state error

ufrshubham · 2022-09-15T04:01:20Z

Title should use feat! as it is a breaking change

ufrshubham

Just a few thoughts to make the changes a little soft for the end users

ufrshubham · 2022-09-15T04:30:12Z

packages/tiled/lib/src/common/property.dart

+
+    return properties.groupFoldBy((prop) => prop.name, (previous, element) {
+      if (previous != null) {
+        throw ArgumentError("Can't have two properties with the same name.");


I don't think we should throw an error here. If Tiled does not even allow creation of multiple properties with same name, this case should ideally never occur. Our input condition is a valid tmx file.

In the rare case someone manually tweaks their tmx and adds multiple properties with same name, instead of failing, we should follow what Tiled does. Tiled loads the map and displays only one of the duplicate properties (the latest that it finds). We can also do that by replacing the old value with latest value when a duplicate key is found.

If we really want to go the extra mile and inform the users, it can be done using an assert.

ufrshubham · 2022-09-15T04:34:01Z

packages/tiled/lib/src/common/property.dart

@@ -33,14 +33,21 @@ class Property {
 }

 extension PropertiesParser on Parser {
-  List<Property> getProperties() {
-    return formatSpecificParsing(
+  Map<String, Property> getProperties() {


Can we minimize the breaking change by adding a new getPropertiesMap() that returns Map<String, Property> and making getProperties() return the values iterable from that map?

I don't think anyone uses this getProperties method directly, but rather the properties public variable in Tile Object Layer etc.

We definitely could change the API to:

class Tile { ... Iterable<Property> get properties => propertiesByName.values; final Map<String, Property> propertiesByName; }

But personally I think that's a worse API and there is really no value in an Iterable<Property> accessor, especially when you can get it via .values.

I was under the impression that we're in early stages with this library and want to do large refactors to polish the API before 1.0 so that's why I was leaning towards the more aggressive refactor. But, I'm happy either way 🤷

I don't think anyone uses this getProperties method directly, but rather the properties public variable in Tile Object Layer etc.

I get what you mean, but this is still just an assumption. If it is a public API, it is always better to assume that it is used by someone. Also, even if getProperties is not used by anyone, we are still breaking properties.

But personally I think that's a worse API and there is really no value in an Iterable<Property> accessor, especially when you can get it via .values.

I was under the impression that we're in early stages with this library and want to do large refactors to polish the API before 1.0 so that's why I was leaning towards the more aggressive refactor. But, I'm happy either way 🤷

@spydon can comment about plans for 1.0, but I feel making a breaking change and trying not to bump up the major version are conflicting goals here. If you make the changes non-breaking, it will automatically solve the versioning problem. A middle ground would be to let getProperties and properties work as they used to with a @deprecated tag saying that it will be removed in 1.0.

*I might have a hidden motive for making it non-breaking, because properties is used in my simple platformer series. It will be better if maintainers decide what to do about this 😅

It doesn't need to have a major bump when it is breaking and sub v1. I think that we can deprecate getProperties, but not remove it until we go to past v1, properties is more inline with the dart naming convention anyways.
So then your tutorials wouldn't break for a long time.

Poke me when you feel that a new tiled + flame_version should be released btw.

So, keep getProperties and properties and add getPropertiesByName and propertiesByName?

To be clear getProperties and properties are two different things, getProperties is the function that parses the data which eventually gets set on the properties class level vars.

Aah right, hard to review PRs properly on the phone. Should getProperties only be used internally, or is there a value in exposing that to the user? If it's not I would name it parseProperties and mark it as @internal.
And for properties I think that we should do a breaking change on the type then and not pollute the code with unnecessary members already.

Sounds good, do you want me to rename ALL the parser methods?

Parser.getString

Parser.getStringOrNull

Parser.getInt

Parser.getColor

etc etc

Ah maybe not, can't come up with any better names for them at least, can you?
Do you know why PropertiesParser is an extensions method and doesn't simply exist on the Parser class instead?

Not sure, I can take a look.

For the method naming, I think the classname makes it self explanatory so I'm inclined to leave it as is, what do you think?

For the method naming, I think the classname makes it self explanatory so I'm inclined to leave it as is, what do you think?

Agree, missed that it was on the parser class when I reviewed on mobile.

kurtome · 2022-09-15T13:41:41Z

Title should use feat! as it is a breaking change

@ufrshubham Doesn't that bump this library to 1.0? I didn't want to keep bumping the major version every time we refactor something now.

spydon · 2022-09-16T06:15:40Z

Title should use feat! as it is a breaking change

@ufrshubham Doesn't that bump this library to 1.0? I didn't want to keep bumping the major version every time we refactor something now.

It won't, sub v1 breaking changes doesn't lead to v1. One is meant to have breaking changes before v1. So it is good to have the ! for clarity when people are reading the changelog since it will organise the breaking changes in its own section.

kurtome · 2022-09-19T05:02:36Z

@spydon @ufrshubham @jtmcdole Not sure about this API, but it has some merit:

bool isFoo = tile.properties.named<BoolProperty>("foo").value;

The benefit here is each Property class can add things, that alternative of making it just Property<T> seemed messier.

jtmcdole · 2022-09-19T05:09:50Z

I guess the question is, does the user care about the property or care about the value ? I.e. <bool> vs boolproperty...value.

jtmcdole · 2022-09-19T05:10:52Z

Is properties not being turned into a map?

kurtome · 2022-09-19T05:52:32Z

I guess the question is, does the user care about the property or care about the value ? I.e. <bool> vs boolproperty...value.

They definitely care more about the value, the question is how to handle complex properties like File and Object and perhaps others with the same API as primitives

They are being read into a Map, but with a wrapper class so we can add convenience methods to it. We could make it indexable though

ufrshubham · 2022-09-19T05:57:13Z

Value for File and Object properties are just string and int respectively. So it is just the custom property types that we need to care about

kurtome · 2022-09-19T06:02:59Z

Value for File and Object properties are just string and int respectively. So it is just the custom property types that we need to care about

We don't want to give them the option of a reference to the Object directly from the property?

kurtome · 2022-09-19T06:05:56Z

If we want to be super minimal we can just give them a map of string/dynamic and make them cast too use the value. Feels gross though

ufrshubham · 2022-09-19T06:07:58Z

We don't want to give them the option of a reference to the Object directly from the property?

We do, but I'd prefer it to be like this:

final isFoo = tile.properties.named<bool>("foo");
final objectOfMyClass = tile.properties.named<MyClass>("bar");

And in the second case, MyClass is expected to handle the string/dynamic map. It will nicely hook into the code generation that we were discussing on discord.

kurtome · 2022-09-19T06:12:06Z

I see, I think the other point @jtmcdole is making is, can we just return the value instead of making them call .value, we should probably support both

ufrshubham · 2022-09-19T06:16:17Z

can we just return the value instead of making them call .value

Yeah, I am in favor of returning directly the value. We can keep .value around if it has some use case that directly returning value does not solve.

ufrshubham · 2022-09-19T07:29:30Z

Here is a rough idea that I had in my mind.

We can have 2 abstract classes for handling/building the custom types created in Tiled

abstract class CustomPropertyType {}

abstract class CustomPropertyBuilder<T extends CustomPropertyType> {
  T build(Property<dynamic> property);
}

Then, in the CustomProperties class, we can add a map that stores all the user defined builders.

class CustomProperties extends Iterable<Property> {
  // omitted code....
  final Map<Type, CustomPropertyBuilder<CustomPropertyType>> builders;

  // Just an example. This method can be moved at a higher level 
  // to make it more accessible to users.
  void register<T extends CustomPropertyType>(
    CustomPropertyBuilder<T> instance,
  ) {
    builders[T.runtimeType] = instance;
  }

  /// Get a typed property by its name
  T named<T>(String name) {
    // If user is requesting a custom property, we look for the registered builder 
    // and delegate the conversion to that builder.
    if (T is CustomPropertyType) {
      final builder = builders[T.runtimeType];
      assert(
        builder != null,
        'No builder found for $T. Did you forgot to register one?',
      );
      return builder!.build(byName[name]!) as T;
    } else {
      return byName[name]! as T;
    }
  }
}

If users want to implement their own builders they can do so. Or else they can use code generation to generate derived classes from CustomPropertyType and CustomPropertyTypeBuilder.

kurtome · 2022-09-19T14:15:07Z

I renamed the methods inside CustomProperties so now getting values simpler:

Accessing an int value

properties.get<int>('foo') == 3

Accessing an int property:

properties.getProp<IntProperty>('foo') == IntProperty(name: 'foo', value: 3);

spydon · 2022-09-19T14:28:07Z

packages/tiled/lib/src/common/property.dart

  /// Get a typed property by its name
-  T named<T extends Property<dynamic>>(String name) {
+  T getProp<T extends Property<dynamic>>(String name) {


We have a ban on abbreviations 😉

Which is why I was saying "as"....

CustomProperties could also just implement:

Property operator[] (String name) => byName[name];

Then you get:

properties['fu']; // Returns IntProperty!

And if you extend Property to have T as<T>() it looks & feels like other code in Flame.

properties['fu'].as<int>(); // Returns integer!

In that case, wouldn't it be smart to not have CustomProperties class at all. The internal byName map will also provide similar API.

properties['fu']; // Returns IntProperty!

Technically, it will return Property. Users will have to manually cast it to IntProperty. In that regard, properties.getProp<IntProperty>('foo') feels much better.

We should also try to think about making it easier for end users to find these APIs. We can document the overloaded [] operator and as<>() as much as we want, but still there will be people who will not read the docs 😅. I feel that explicitly defined methods are easier to find because of code completion in IDEs.

I think the overloaded operator looks better, I don't think users will have much more of a problem understanding that one than to see that there is the possibility to put generics on getProp (which should be named getProperty if we decide to go with that API anyways).

Is it it possible to put generics on the operator instead of using as?

In that case, wouldn't it be smart to not have CustomProperties class at all. The internal byName map will also provide similar API.

properties['fu']; // Returns IntProperty!

Technically, it will return Property. Users will have to manually cast it to IntProperty. In that regard, properties.getProp<IntProperty>('foo') feels much better.

I think the idea is that as<int> could be called on the base class so you don't have to cast the property first

We should also try to think about making it easier for end users to find these APIs. We can document the overloaded [] operator and as<>() as much as we want, but still there will be people who will not read the docs 😅. I feel that explicitly defined methods are easier to find because of code completion in IDEs.

This was my primary reason for not just using Map style interface in the first place, but we could support both. However, `Property.as‘ returning the value itself feels like it's mixing levels of abstraction, it's operating on the value not in the property, is that weird?

Operator[] is more natural to me. It can't have genetics to my knowledge.

Operator[] retiring the super instance property and the user casting to IntProperty (or runtime testing) is also pretty natural. This also grants access to the new custom classes in unparsed form maybe?

as I feel belongs to property and not the map holding the properties.

At that point, why not just properties["foo"].value as int why have a special method?

kurtome · 2022-09-20T00:31:38Z

Here is a rough idea that I had in my mind.

We can have 2 abstract classes for handling/building the custom types created in Tiled

abstract class CustomPropertyType {}



abstract class CustomPropertyBuilder<T extends CustomPropertyType> {

  T build(Property<dynamic> property);

}

Then, in the CustomProperties class, we can add a map that stores all the user defined builders.

class CustomProperties extends Iterable<Property> {

  // omitted code....

  final Map<Type, CustomPropertyBuilder<CustomPropertyType>> builders;



  // Just an example. This method can be moved at a higher level 

  // to make it more accessible to users.

  void register<T extends CustomPropertyType>(

    CustomPropertyBuilder<T> instance,

  ) {

    builders[T.runtimeType] = instance;

  }



  /// Get a typed property by its name

  T named<T>(String name) {

    // If user is requesting a custom property, we look for the registered builder 

    // and delegate the conversion to that builder.

    if (T is CustomPropertyType) {

      final builder = builders[T.runtimeType];

      assert(

        builder != null,

        'No builder found for $T. Did you forgot to register one?',

      );

      return builder!.build(byName[name]!) as T;

    } else {

      return byName[name]! as T;

    }

  }

}

If users want to implement their own builders they can do so. Or else they can use code generation to generate derived classes from CustomPropertyType and CustomPropertyTypeBuilder.

This is compelling, although I can't help but think we should be using the property's inherent type somehow. Also would be good to not need to rebuild the result every find it's accessed

ufrshubham · 2022-09-20T02:37:31Z

This is compelling, although I can't help but think we should be using the property's inherent type somehow.

But how do we know what is the inherent type of a custom type that a user has defined in Tiled?

Also would be good to not need to rebuild the result every find it's accessed

This could be solved by caching the custom property inside CustomProperties once it is built.

But obviously, these changes are not required to be made in this PR. That would be a different PR for adding support for custom types. Yours originally was just for converting a list to a map.

kurtome · 2022-09-20T14:21:56Z

@spydon @ufrshubham @jtmcdole I renamed the methods to: getValue<T> and getProperty<T>.

Also, I added ["name"] accessor which returns a Property<dynamic>.

I think for now we should roll with this, and then subsequent PRs can continue fiddling with the API? I'm not opposed to any of the suggestions above.

spydon · 2022-09-20T14:30:25Z

Also, I added ["name"] accessor which returns a Property<dynamic>.

Afaik one should pretty much never use dynamic nowadays, but instead use Object?.

kurtome · 2022-09-20T15:00:25Z

Also, I added ["name"] accessor which returns a Property<dynamic>.

Afaik one should pretty much never use dynamic nowadays, but instead use Object?.

Hm, I tried to update everywhere this was using dynamic and I started getting compile errors in other places, for example:

Also, making the return values nullable and adding bool has(String name)

spydon · 2022-09-20T19:25:49Z

Hm, I tried to update everywhere this was using dynamic and I started getting compile errors in other places, for example:

Also, making the return values nullable and adding bool has(String name)

Ah properties is a map from string to dynamic? Maybe that could be changed later.

packages/tiled/lib/src/common/property.dart

kurtome · 2022-09-20T22:48:33Z

Also, I added ["name"] accessor which returns a Property<dynamic>.

Afaik one should pretty much never use dynamic nowadays, but instead use Object?.

Hm, I tried to update everywhere this was using dynamic and I started getting compile errors in other places, for example:

Also, making the return values nullable and adding bool has(String name)

kurtome · 2022-09-20T22:48:46Z

Oops

kurtome · 2022-09-20T22:49:58Z

Ah properties is a map from string to dynamic? Maybe that could be changed later.

Fixed, the problem was Property.parse was returning Property which default the generic to dynamic, so now it's returning Property<Object>

spydon

Just a few small comments, but other than those lgtm!

packages/tiled/lib/src/common/property.dart

spydon · 2022-09-21T12:45:38Z

packages/tiled/lib/src/common/property.dart

+  }
+
+  /// Get a typed property by its name
+  T? getProperty<T extends Property<Object>>(String name) {


Suggested change

T? getProperty<T extends Property<Object>>(String name) {

T? getProperty<T extends Property<T>>(String name) {

T is the type of the property here, not the value, so this change would require that the Property has a value of Property recursively. We'd have to add a second generic to the method, K, which would be awkward:
getProperty<IntProperty, int>('foo')

Ah, of course. Wouldn't one want to just want to specify the type of the value instead, and then the return type can be Property<T>??

Well the intent was to make it simple to get an IntProperty or ColorProperty, otherwise you still have to cast it to one of those if you want the actual property type, which would be more relevant if the property has unique methods on it. Otherwise there is really no reason to be grabbing the property itself at all, you can just grab the value directly

Hmm, what is the real point of having the property wrapper actually? Can't we just offer extensions on the value type instead? Like toHex on Color for example?

We may not need it, but it's not entirely clear to me that every property can just be a simple name/value pair where the value doesn't need custom behavior, if so we should probably ditch the entire Property class

For all the value based use cases your describing, that's what getValue<T> is for, right?

So, are there any usecases for having this wrapper class? @ufrshubham @jtmcdole

I'll also say that there is some value to me to making the class structure match the Tiled data structures for consistency with Tiled itself. Also, we really do want to know if a property is a file string or a text string, etc... which is an argument for keeping the Property class

packages/tiled/lib/src/common/property.dart

spydon

I think this has been sitting and waiting for more reviews for too long now, I'll merge it.
If anything comes up it can be changed in another PR.

feat: turning List<Property> into Map<String, Property>

fc870d0

jtmcdole reviewed Sep 15, 2022

View reviewed changes

ufrshubham reviewed Sep 15, 2022

View reviewed changes

kurtome changed the title ~~feat: turning List<Property> into Map<String, Property>~~ feat!: turning List<Property> into Map<String, Property> Sep 16, 2022

Moving typed properties into CustomProperties class

c2f2161

kurtome changed the title ~~feat!: turning List<Property> into Map<String, Property>~~ feat!: turning List<Property> into CustomProperties Sep 19, 2022

cleaning up properties APi

fe4b2f2

spydon reviewed Sep 19, 2022

View reviewed changes

renaming methods and adding [] operator

5f948d3

fixing nullability

abbad17

spydon reviewed Sep 20, 2022

View reviewed changes

packages/tiled/lib/src/common/property.dart Outdated Show resolved Hide resolved

removing dynamic type and fixing nullability

b80256c

kurtome closed this Sep 20, 2022

kurtome reopened this Sep 20, 2022

spydon requested review from jtmcdole and ufrshubham September 21, 2022 12:40

spydon approved these changes Sep 21, 2022

View reviewed changes

kurtome and others added 3 commits September 21, 2022 09:41

fixing doc comment

d2f0656

Merge branch 'main' into custom-properties-map

c0a51a8

Merge branch 'main' into custom-properties-map

1317008

spydon approved these changes Oct 31, 2022

View reviewed changes

spydon enabled auto-merge (squash) October 31, 2022 15:39

spydon merged commit 51d59ec into flame-engine:main Oct 31, 2022

	T? getProperty<T extends Property<Object>>(String name) {
	T? getProperty<T extends Property<T>>(String name) {

feat!: turning List<Property> into CustomProperties #55

feat!: turning List<Property> into CustomProperties #55

Conversation

kurtome commented Sep 15, 2022 • edited Loading

Description

Checklist

Breaking Change

Migration instructions

Related Issues

jtmcdole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ufrshubham commented Sep 15, 2022

ufrshubham left a comment

Choose a reason for hiding this comment

ufrshubham Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

ufrshubham Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ufrshubham Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtome commented Sep 15, 2022

spydon commented Sep 16, 2022

kurtome commented Sep 19, 2022

jtmcdole commented Sep 19, 2022

jtmcdole commented Sep 19, 2022

kurtome commented Sep 19, 2022

ufrshubham commented Sep 19, 2022

kurtome commented Sep 19, 2022

kurtome commented Sep 19, 2022

ufrshubham commented Sep 19, 2022 • edited Loading

kurtome commented Sep 19, 2022

ufrshubham commented Sep 19, 2022

ufrshubham commented Sep 19, 2022

kurtome commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtome commented Sep 20, 2022

ufrshubham commented Sep 20, 2022 • edited Loading

kurtome commented Sep 20, 2022

spydon commented Sep 20, 2022

kurtome commented Sep 20, 2022

spydon commented Sep 20, 2022

kurtome commented Sep 20, 2022

kurtome commented Sep 20, 2022

kurtome commented Sep 20, 2022 • edited Loading

spydon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtome Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtome Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

spydon left a comment

Choose a reason for hiding this comment

kurtome commented Sep 15, 2022 •

edited

Loading

ufrshubham Sep 15, 2022 •

edited

Loading

ufrshubham Sep 15, 2022 •

edited

Loading

ufrshubham Sep 15, 2022 •

edited

Loading

ufrshubham commented Sep 19, 2022 •

edited

Loading

ufrshubham commented Sep 20, 2022 •

edited

Loading

kurtome commented Sep 20, 2022 •

edited

Loading

kurtome Sep 22, 2022 •

edited

Loading

kurtome Sep 23, 2022 •

edited

Loading