Added AliasProperty and tests #10
Conversation
|
I like the concept, and the updated introspection in Do you mind removing the unrelated changes from this PR though? Your code editor included quite a bit of PEP8 corrections and while I don't disagree with a little linting, I'd rather it be separate from this change (makes it less clear what you've actually altered). |
About the linting I apologize I even read your contributing guide and was trying to keep things styled the same. PyCharm automatically refactors the code and I forgot to disable that. I'll remove the linting applied by PyCharm and update the pull request. |
| class AliasProperty(Property): | ||
| """Property with a getter method and optional setter method. Behaves similar to Pythons builtin properties. | ||
|
|
||
| Args: |
There was a problem hiding this comment.
Can you add a description for bind here?
| self.__setter = setter | ||
| self.__bindings = dict((prop, self._on_change) for prop in bind) if bind is not None else {} | ||
|
|
||
| def _on_change(self, obj, *args, **kwargs): |
There was a problem hiding this comment.
I'm wondering if it would be better to use a separate callback for bound properties instead of overriding _on_change.
Seems like there could be a lot of added method calls with all of the extra bindings. It would also be cleaner without having to inspect the source Property, calls to super(), etc.
There was a problem hiding this comment.
I agree I originally added an _trigger_change callback. But I didn't want to add to much bloat. I wanted to keep additions limited.
There was a problem hiding this comment.
I think an additional method specific to AliasProperty would be just fine. Doesn't really add bloat and would make intent much clearer.
Maybe something like _on_bound_prop_change? I'm not the best at naming things, but you get the idea.
| class A(Dispatcher): | ||
| test_prop = Property('default') | ||
| name = Property('') | ||
| something = Property() | ||
|
|
||
| def get_name_something(self): |
There was a problem hiding this comment.
Can you also add a test for the setter?
There was a problem hiding this comment.
Can you also add a test for the
setter?
I must of forgot to commit this. I already have the setter added here locally I'll fix the other issues and re-commit.
No worries. I also noticed you're working off of the master branch of your fork. This will cause headaches for you later on if you try to keep things in sync with this repo (I made the same mistake for my first PR). Not sure if you want to deal with that now (would be easier for you, but it'd be a separate PR) or later (would require some |
Added an
AliasPropertysimilar to Kivy but without thekvlanguage specific attributes. This required refactoring the__new__method of theDispatcherclass. The bind argument to AliasProperty requires properties to_add_instancein declaration order. The builtindir()method doesn't list attributes in declaration order. Now the__dict__property of the class is used for detecting properties. I also changed the iteration of base classes to use the__mro__since that is the correct inheritance order already unraveled.