-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat!: turning List<Property> into CustomProperties #55
Changes from 1 commit
fc870d0
c2f2161
fe4b2f2
5f948d3
abbad17
b80256c
d2f0656
c0a51a8
1317008
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,14 +33,21 @@ class Property { | |
} | ||
|
||
extension PropertiesParser on Parser { | ||
List<Property> getProperties() { | ||
return formatSpecificParsing( | ||
Map<String, Property> getProperties() { | ||
final properties = formatSpecificParsing( | ||
(json) => json.getChildrenAs('properties', Property.parse), | ||
(xml) => | ||
xml | ||
.getSingleChildOrNull('properties') | ||
?.getChildrenAs('property', Property.parse) ?? | ||
[], | ||
); | ||
|
||
return properties.groupFoldBy((prop) => prop.name, (previous, element) { | ||
if (previous != null) { | ||
throw ArgumentError("Can't have two properties with the same name."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a state error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should throw an error here. If 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. |
||
} | ||
return element; | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we minimize the breaking change by adding a new
getPropertiesMap()
that returnsMap<String, Property>
and makinggetProperties()
return thevalues
iterable from that map?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think anyone uses this
getProperties
method directly, but rather theproperties
public variable inTile
Object
Layer
etc.We definitely could change the API to:
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 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 breakingproperties
.@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
andproperties
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 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, keep
getProperties
andproperties
and addgetPropertiesByName
andpropertiesByName
?To be clear
getProperties
andproperties
are two different things,getProperties
is the function that parses the data which eventually gets set on theproperties
class level vars.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 itparseProperties
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, do you want me to rename ALL the parser methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theParser
class instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, missed that it was on the parser class when I reviewed on mobile.