Add MetadataFilter for observing traits using metadata #1095
Conversation
traits/observers/_metadata_filter.py
Outdated
| if self.metadata_name in trait.__dict__: | ||
| value = getattr(trait, self.metadata_name) | ||
| return self.value(value) | ||
| return False |
There was a problem hiding this comment.
This value filter is more than necessary for supporting the "+metadata_name" feature in the mini-language.
On the one hand, it is easy to implement, and is consistent with HasTraits.traits support on filtering metadata using the values. On the other hand, this is a feature I am not sure if we have a use case for (but maybe we do, they don't exist because users have been working around its nonexistence.)
Furthermore, the filtering here is not the exactly same as that in HasTraits.traits: HasTraits.traits does not differentiate when a metadata is defined or not. If the metadata is undefined, it assumes it existed with None as the value and passes it to the filter and I don't think we wanted the same behaviour here.
There was a problem hiding this comment.
We could remove this value and deem this MetadataFilter for checking for metadata existence only, for now. If one really needs to filter the metadata value, filter_(lambda name, trait: trait.my_metadata_name is not None and trait.my_metadata_name > 10.0) can still be defined for that use case (note that is not None check), and if it becomes common enough, adding this value filter back is easy.
There was a problem hiding this comment.
Agreed; that sounds good to me.
There was a problem hiding this comment.
value is removed. This is now ready for review again.
kitchoi
changed the title
kitchoi
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1095 +/- ##
==========================================
- Coverage 76.15% 74.24% -1.92%
==========================================
Files 54 79 +25
Lines 6493 8479 +1986
Branches 1263 1610 +347
==========================================
+ Hits 4945 6295 +1350
- Misses 1205 1806 +601
- Partials 343 378 +35
Continue to review full report at Codecov.
|
traits/observers/_metadata_filter.py
Outdated
| self.metadata_name = metadata_name | ||
|
|
||
| def __call__(self, name, trait): | ||
| return self.metadata_name in trait.__dict__ |
There was a problem hiding this comment.
I don't think this is the right test. What's in the __dict__ is an implementation detail; what matters is what happens on metadata retrieval.
For example, I'd expect the following two traits to behave the same under any metadata filtering:
>>> tr1 = Float(2.3, edible=None).as_ctrait()
>>> tr2 = Float(2.3).as_ctrait()
>>> tr2.edible == tr1.edible
True
So I think we want to retrieve getattr(trait, metadata_name) and then filter on the result. What's not clear to me is whether that filtering should be is not None or bool. What does the usual on_trait_change expression do with this?
There was a problem hiding this comment.
Ah, looks like this is the relevant code. So we want an is not None test.
There was a problem hiding this comment.
Okay. I thought the behaviour of returning None for an undefined metadata is conflating the concept of something being undefined VS something being defined with a value of None and here we only want to check for metadata existence. So it seems that the fact that these two concepts being combined is in fact the desired behaviour for metadata?
There was a problem hiding this comment.
Not sure about "desired", but it is the current design. Changing that design would be an orthogonal discussion, I think.
There was a problem hiding this comment.
I see. Thanks. I will add a comment because this current design is still strange to me, not sure if I am alone in that.
This PR targets this item in #977:
MetadataFilterto be used as thefiltercallable inFilteredTraitObserver. Users typically create these observers via text"+metadata", and a newfilterinstance will be created. The reason why we need a separate class for this filter is for supporting equality check on the filter when the user requests an existing observer to be removed.TestWithFilteredTraitObserverfor how this is used with an instance ofHasTraits.Checklist
Update API reference (docs/source/traits_api_reference)Update User manual (docs/source/traits_user_manual)Update type annotation hints intraits-stubs