Add trait_added support for observing a named trait #1077

kitchoi · 2020-05-12T15:49:52Z

This PR implements item 11 of #977

Add an TraitAddedObserver for supporting other observers that need to handle trait_added. - Use TraitAddedObserver in NamedTraitObserver.

Checklist

Tests
~~Update API reference (docs/source/traits_api_reference)~~ this observer is for internal use.
~~Update User manual (docs/source/traits_user_manual)~~
~~Update type annotation hints in traits-stubs~~

kitchoi · 2020-05-12T15:52:11Z

traits/observers/_testing.py

@@ -47,9 +51,6 @@ def call_add_or_remove_notifiers(**kwargs):
    **kwargs
        New argument values to use instead.
    """
-    def dispatch_same(handler, event):
-        handler(event)
-


For tests trying to remove notifiers, notifiers with different dispatcher are considered different. Instead of keep supplying a locally defined dispatcher to maintain equality, it is convenient to just use a constant one.

codecov-io · 2020-05-12T17:52:21Z

Codecov Report

Merging #1077 into master will decrease coverage by 2.12%.
The diff coverage is 62.24%.

@@            Coverage Diff             @@
##           master    #1077      +/-   ##
==========================================
- Coverage   76.15%   74.03%   -2.13%     
==========================================
  Files          54       76      +22     
  Lines        6493     8415    +1922     
  Branches     1263     1597     +334     
==========================================
+ Hits         4945     6230    +1285     
- Misses       1205     1807     +602     
- Partials      343      378      +35

Impacted Files	Coverage Δ
traits/api.py	`100.00% <ø> (+9.67%)`	⬆️
traits/base_trait_handler.py	`61.76% <ø> (ø)`
traits/ctrait.py	`71.54% <ø> (+0.47%)`	⬆️
traits/has_traits.py	`72.49% <ø> (-0.28%)`	⬇️
traits/observers/_i_notifier.py	`0.00% <0.00%> (ø)`
traits/observers/events.py	`0.00% <0.00%> (ø)`
traits/traits.py	`75.10% <ø> (-2.45%)`	⬇️
traits/util/resource.py	`15.25% <ø> (ø)`
traits/observers/_generated_parser.py	`51.96% <51.96%> (ø)`
traits/trait_types.py	`72.29% <80.00%> (-0.16%)`	⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 240d469...3eb6027. Read the comment docs.

… observer should not be mutated despite this change

mdickinson · 2020-05-18T15:49:18Z

traits/observers/_trait_added_observer.py

+
+    Parameters
+    ----------
+    match_func : callable(str, TraitType) -> boolean


TraitType doesn't seem like the right thing here. (We haven't got rid of TraitHandler yet, so not all of our traits even have an associated TraitType.)

I think I'd expect to receive the actual trait (so an instance of CTrait) instead.

Nitpick:

Suggested change

match_func : callable(str, TraitType) -> boolean

match_func : callable(str, TraitType) -> bool

Thanks! This is a mistake indeed. Fixed.

mdickinson

Mostly LGTM; a couple of nitpicks, and one point of clarification on TraitType versus CTrait needed.

mdickinson · 2020-05-18T15:53:59Z

traits/observers/_trait_added_observer.py

+
+    Parameters
+    ----------
+    match_func : callable(str, TraitType) -> boolean


Nitpick:

Suggested change

match_func : callable(str, TraitType) -> boolean

match_func : callable(str, TraitType) -> bool

mdickinson · 2020-05-18T15:55:58Z

traits/observers/_trait_added_observer.py

+                return
+            raise ValueError(
+                "Unable to observe 'trait_added' event on {!r}".format(object))
+        yield object._trait("trait_added", 2)


Those magic integers ("2") are killing me. But that's out of scope for this PR.

mdickinson · 2020-05-18T15:57:48Z

traits/observers/_trait_added_observer.py

+            dispatcher=dispatcher,
+        )
+
+    def prevent_event(self, event):


The logical negation in this method implementation is making me wonder whether we should switch from prevent_event to accept_event to avoid the extra negative (both in the code, and conceptually). Are we seeing similar negations in other implementations of prevent_event?

That's orthogonal to this PR, of course.

Most of the prevent_event are lambda event: False, a few of them are lambda event: event.old is Uninitialized. Switching this to accept_event would also look fine for those.

mdickinson · 2020-05-18T15:58:49Z

traits/observers/_trait_added_observer.py

+        """
+        object = event.object
+        name = event.new
+        trait = object.trait(name=name)


Ah, following on from my earlier comment, it looks as though we're passing a CTrait rather than TraitType here. I think the CTrait instance is what I'd expect.

mdickinson · 2020-05-18T16:01:58Z

traits/observers/tests/test_trait_added_observer.py

+        return hash(self.return_value)
+
+
+class TestTraitAddedObserverEqualHashImmutable(unittest.TestCase):


That's an impressive test class name ...

Hmmm the "Immutable" part needs to go, removing...

…ency

traits/observers/_trait_added_observer.py

mdickinson

LGTM

Co-authored-by: Mark Dickinson <mdickinson@enthought.com>

codecov-commenter · 2020-05-19T08:20:19Z

Codecov Report

Merging #1077 into master will decrease coverage by 1.94%.
The diff coverage is 62.31%.

@@            Coverage Diff             @@
##           master    #1077      +/-   ##
==========================================
- Coverage   76.15%   74.21%   -1.95%     
==========================================
  Files          54       78      +24     
  Lines        6493     8469    +1976     
  Branches     1263     1610     +347     
==========================================
+ Hits         4945     6285    +1340     
- Misses       1205     1806     +601     
- Partials      343      378      +35

Impacted Files	Coverage Δ
traits/api.py	`100.00% <ø> (+9.67%)`	⬆️
traits/base_trait_handler.py	`61.76% <ø> (ø)`
traits/ctrait.py	`72.35% <ø> (+1.28%)`	⬆️
traits/observers/_i_notifier.py	`0.00% <0.00%> (ø)`
traits/observers/events.py	`0.00% <0.00%> (ø)`
traits/traits.py	`75.10% <ø> (-2.45%)`	⬇️
traits/util/resource.py	`15.25% <ø> (ø)`
traits/observers/_generated_parser.py	`51.96% <51.96%> (ø)`
traits/has_traits.py	`73.51% <64.70%> (+0.74%)`	⬆️
traits/trait_types.py	`72.29% <80.00%> (-0.16%)`	⬇️
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 240d469...f6f1a7b. Read the comment docs.

kitchoi added 4 commits May 12, 2020 16:26

Add trait_added support for NamedTraitObserver

029cd9c

Fix docstring

3303684

Add comment related to #1047

739eaa0

Remove redundant get_notifier

bdd6a24

kitchoi commented May 12, 2020

View reviewed changes

kitchoi added 2 commits May 12, 2020 16:54

Fix comment about remove_trait

18b8287

Differentiate test failure and test error

ee0c840

This was referenced May 12, 2020

Add an observer for observing traits using an arbitrary filter #1078

Merged

Implement new framework to observe and notify for trait changes #977

Closed

Minor improvement to docstring

c980b1f

kitchoi added 3 commits May 12, 2020 18:59

Extend unit tests for _RestrictedNamedTraitObserver

dd350df

Relax immutability check for consistencies with other observers. This…

46d85ec

… observer should not be mutated despite this change

Use name in hash

8852d92

mdickinson reviewed May 18, 2020

View reviewed changes

mdickinson requested changes May 18, 2020

View reviewed changes

mdickinson reviewed May 18, 2020

View reviewed changes

kitchoi added 5 commits May 18, 2020 17:47

Fix test case name

d88d481

Fix docstring for match_func

b60ffc3

Ditch immutability for _RestrictedNamedTraitObserver.name for consist…

2e71765

…ency

Import order

2e31ade

Import order

3eb6027

kitchoi requested a review from mdickinson May 18, 2020 16:57

mdickinson reviewed May 19, 2020

View reviewed changes

traits/observers/_trait_added_observer.py Outdated Show resolved Hide resolved

mdickinson approved these changes May 19, 2020

View reviewed changes

Update traits/observers/_trait_added_observer.py

f6f1a7b

Co-authored-by: Mark Dickinson <mdickinson@enthought.com>

kitchoi merged commit 486a289 into master May 19, 2020

kitchoi deleted the 977-trait-added-observer branch May 19, 2020 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add trait_added support for observing a named trait #1077

Add trait_added support for observing a named trait #1077

kitchoi commented May 12, 2020

kitchoi May 12, 2020

codecov-io commented May 12, 2020 •

edited

Loading

mdickinson May 18, 2020

mdickinson May 18, 2020

kitchoi May 18, 2020

mdickinson left a comment

mdickinson May 18, 2020

mdickinson May 18, 2020

mdickinson May 18, 2020

kitchoi May 18, 2020

mdickinson May 18, 2020

mdickinson May 18, 2020

kitchoi May 18, 2020

mdickinson left a comment

codecov-commenter commented May 19, 2020 •

edited

Loading

	match_func : callable(str, TraitType) -> boolean
	match_func : callable(str, TraitType) -> bool

		return hash(self.return_value)


		class TestTraitAddedObserverEqualHashImmutable(unittest.TestCase):

Add trait_added support for observing a named trait #1077

Add trait_added support for observing a named trait #1077

Conversation

kitchoi commented May 12, 2020

Choose a reason for hiding this comment

codecov-io commented May 12, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 19, 2020 • edited Loading

Codecov Report

codecov-io commented May 12, 2020 •

edited

Loading

codecov-commenter commented May 19, 2020 •

edited

Loading