register TraitEventHandlers with tags #230

rmorshea · 2016-05-23T03:55:33Z

Allows the use of metadata as a way to register trait events

class A(HasTraits):
    foo = Int().tag(bar=True)
    @observe(tags={'bar':True})
    def _bar_observer(self, change):
        print 'change from bar tagged trait'

a = A()

a.foo = 1

The same pattern also works for @validate and @default.

General:

More elegant than Observe Traits Via Metadata #111.
Closes don't use assert. #219, observe should allow any iterable containers. #215, observe should raise with better error if wrong argument passed. #221

Under The Hood:

default, validate, observe (method and decorator), unobserve, and unobserve_all have a new 'tags' keyword that provides metadata for gathering groups of traits for observation or for removing observers (depending on the method).
- The metadata provided via 'tags' does not create a dynamic reference to traits - e.g. if the metadata of a trait is modified, the tagged registration of observers to that trait will not be recomputed. Thus, in edge cases where metadata might be modified, or new traits are added, the timing of a call to one of the above methods should be closely considered.
New functionality for unobserve_all
- Previously unobserve_all either deleted all notifiers, or all notifiers of a given name.
- Now, it's taken a feature previously belonging to HasTraits.unobserve when its 'handler' argument was None. Thus unobserve_all can remove the handlers of a given name and type.
- Additionally, unobserve_all can now remove all handlers of a specified type across all traits.
- Calls to unobserve where 'handler' is None are redirected to unobserve_all with a warning.

minrk · 2016-05-31T11:35:04Z

traitlets/traitlets.py

@@ -1199,7 +1327,7 @@ def on_trait_change(self, handler=None, name=None, remove=False):
        else:
            self.observe(_callback_wrapper(handler), names=name)

-    def observe(self, handler, names=All, type='change'):
+    def observe(self, handler, names=None, tags=None, type='change'):


The insertion of an argument makes this a backward-incompatible change.

@minrk I've inserted a warning, so it could be phased in. Should it be moved after type though? The order wouldn't really make sense, but it would make it compatible.

Yeah, I'd get in the habit of putting new args at the end. Most of these should be treated (in documentation, etc.) as keyword-only, even if they can be positional.

rmorshea · 2016-06-21T21:28:29Z

@minrk waiting on #240

Carreau · 2016-06-22T18:52:55Z

traitlets/traitlets.py

-        self.trait_names = names
+    caches_instance_resources = True
+
+    def __init__(self, names, tags, type):


Change of API also, types moves from 3rd to 4th.

This has been changed now.

Carreau · 2016-06-22T22:12:31Z

@rmorshea Sorry if I can seem annoying, I understand that I might be looking like I'm bikeshedding.
Though, I have been confronted to this kind of things before. These are things that I learned the hard way, and these are difficult things to get use to.

Also @rmorshea You are doing a fantastic job, and understand that if we reach this kind of level of review in a pull request is because you are doing really great, so we are pushing you into even harder things !

I hope you are not taking anything personally, and if you find there is too much pressure, or that we are too hard, feel free to ask for help. I know it can be hard to have a PR languishing for a long time.

rmorshea · 2016-06-22T22:21:05Z

@Carreau I totally understand. The fact that I'm not working heavily with traitlets myself means I might not have the greatest foresight for the problems that will crop up in the future. So I value whatever input you or anyone else has.

rmorshea · 2016-06-22T22:22:47Z

... adding tests for @validate and @default via tags

Carreau · 2016-06-22T22:24:20Z

Don't worry that's something you'll learn through time !

minrk · 2016-07-06T10:51:25Z

traitlets/traitlets.py

@@ -820,10 +846,10 @@ def validate(*names):
    exiting the ``hold_trait_notifications` context, and such changes may not
    commute.
    """
-    return ValidateHandler(names)
+    return ValidateHandler(names, tags=kwargs.get('tags', {}))


Should probably raise on unrecognized kwargs

minrk · 2016-07-06T10:55:17Z

should HasTraits.trait_events return these static handlers in addition to the methods it already does?

I don't think there is a reason to distinguish between @observe and observe. What prompted you to maintain this distinction? When would it be relevant?

rmorshea · 2017-02-01T06:57:46Z

@minrk rebased. Not sure how trivial the hit to patch coverage is yet...

rmorshea · 2017-03-16T19:19:53Z

@minrk, any chance we can fast-track a review of this PR? I'm hoping to integrate this functionality in my work for matplotlib soon.

ankostis · 2017-03-16T19:35:58Z

In general it would be nice to start having some 5.x.x pre-releases, of possible.

minrk · 2017-03-17T09:36:14Z

traitlets/traitlets.py

@@ -820,7 +840,7 @@ def compatible_observer(self, change_or_name, old=Undefined, new=Undefined):
    return compatible_observer


-def validate(*names):
+def validate(*names, **kwargs):


Why **kwargs instead of explict tags=None?

The signature is the same as observe because both validators and default generators can be registered to multiple traits via *names or tags=<metadata>. So to mirror the usage of observe all keywords need to be captured and handled in **kwargs.

minrk · 2017-03-17T09:39:47Z

traitlets/traitlets.py

-        for n in names:
-            self._remove_notifiers(handler, n, type)
+        if isinstance(tags, six.string_types):
+            warn("new argument 'tags' introduced: use 'type' as keyword",


since tags didn't get inserted before type, I'm not sure why this warning.

Yes, this should be removed.

minrk · 2017-03-17T09:41:30Z

traitlets/traitlets.py

@@ -995,6 +1066,7 @@ def setup_instance(*args, **kwargs):
        self._trait_values = {}
        self._trait_notifiers = {}
        self._trait_validators = {}
+        self._static_trait_notifiers = []


What is the new _static_trait_notifiers for?

I'm working on a rewrite to remove this. It has to do with complications associated with re-registering tagged notifiers created with HasTraits.observe to new traits created via add_traits.

minrk · 2017-03-17T09:42:02Z

traitlets/traitlets.py

+        events.extend(self.trait_events().values())
+
+        for e in events:
+            if e.caches_instance_resources:


What does e.caches_instance_resources mean?

Similarly to _static_trait_notifiers I need to rethink this and make the implementation cleaner.

rmorshea · 2017-03-21T23:01:55Z

@minrk, I've drastically simplified things. Still need to review the accuracy of the docstrings, and edit this PR's description to reflect changes.

rmorshea · 2017-04-19T19:22:46Z

After much simplifying, this should be ready to be merged.

cc: @minrk

…ervation

rmorshea · 2017-08-14T23:37:39Z

I think we can push this back to 5.1 since it's not a breaking change.

Carreau · 2020-05-31T05:22:10Z

Closing as state for 3 years. Labeled accordingly, feel free to reopen.

rmorshea force-pushed the tagged_events branch 4 times, most recently from 700eb03 to 48bff94 Compare May 23, 2016 05:57

minrk reviewed May 31, 2016
View reviewed changes

rmorshea force-pushed the tagged_events branch from 48bff94 to 29470fe Compare June 3, 2016 21:31

Carreau reviewed Jun 22, 2016
View reviewed changes

rmorshea force-pushed the tagged_events branch 2 times, most recently from 7907497 to ee9421e Compare June 22, 2016 21:02

Carreau mentioned this pull request Jun 23, 2016

Command line flag do not take precedence over config. #248

Closed

rmorshea force-pushed the tagged_events branch 7 times, most recently from fc33fd8 to b4c9131 Compare July 1, 2016 16:37

rmorshea mentioned this pull request Jul 1, 2016

better errors, type checking in event handlers #253

Merged

minrk reviewed Jul 6, 2016
View reviewed changes

rmorshea mentioned this pull request Jul 11, 2016

how to extend dynamic defaults with super #256

Closed

minrk modified the milestone: 5.0 Jul 29, 2016

rmorshea force-pushed the tagged_events branch 2 times, most recently from 1431e09 to 751b750 Compare February 1, 2017 06:48

minrk reviewed Mar 17, 2017

View reviewed changes

rmorshea force-pushed the tagged_events branch from d8914d1 to c8e4660 Compare March 21, 2017 22:55

rmorshea added 5 commits March 21, 2017 15:55

register TraitEventHandlers with tags

ab590f2

various compat changes

6b7d593

misc and tests for validate and default via tags

0460b6d

update following rebase

d758882

changes to tagged event handlers following review

782f695

rmorshea force-pushed the tagged_events branch from c8e4660 to 782f695 Compare March 21, 2017 22:59

rmorshea added 2 commits March 22, 2017 00:36

remove the fluff: TraitEventHandler and hash

80b150f

combine notifier parse functions

595cbf4

rmorshea force-pushed the tagged_events branch from 389ddcf to 4afcd16 Compare April 19, 2017 19:11

correct docstring and add warning to unobserve

1965fb6

rmorshea force-pushed the tagged_events branch from 4afcd16 to 1965fb6 Compare April 19, 2017 19:20

fix parse_notifier_names bug and remove new trait test for tagged obs…

9033f83

…ervation

rmorshea modified the milestones: 5.1, 5.0 Aug 14, 2017

Carreau added the Closed PR Stalled PR, feel free to resurrect. label May 31, 2020

Carreau closed this May 31, 2020

Carreau modified the milestones: 5.1, no action Aug 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

register TraitEventHandlers with tags #230

register TraitEventHandlers with tags #230

rmorshea commented May 23, 2016 •

edited

minrk May 31, 2016

rmorshea May 31, 2016 •

edited

minrk Jun 22, 2016

rmorshea commented Jun 21, 2016

Carreau Jun 22, 2016

rmorshea Jun 22, 2016 •

edited

Carreau commented Jun 22, 2016

rmorshea commented Jun 22, 2016

rmorshea commented Jun 22, 2016

Carreau commented Jun 22, 2016

minrk Jul 6, 2016

minrk commented Jul 6, 2016

rmorshea commented Feb 1, 2017

rmorshea commented Mar 16, 2017

ankostis commented Mar 16, 2017

minrk Mar 17, 2017

rmorshea Mar 20, 2017 •

edited

minrk Mar 17, 2017

rmorshea Mar 20, 2017

minrk Mar 17, 2017

rmorshea Mar 20, 2017 •

edited

minrk Mar 17, 2017

rmorshea Mar 20, 2017

rmorshea commented Mar 21, 2017

rmorshea commented Apr 19, 2017

rmorshea commented Aug 14, 2017

Carreau commented May 31, 2020

register TraitEventHandlers with tags #230

register TraitEventHandlers with tags #230

Conversation

rmorshea commented May 23, 2016 • edited

General:

Under The Hood:

Choose a reason for hiding this comment

rmorshea May 31, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmorshea commented Jun 21, 2016

Choose a reason for hiding this comment

rmorshea Jun 22, 2016 • edited

Choose a reason for hiding this comment

Carreau commented Jun 22, 2016

rmorshea commented Jun 22, 2016

rmorshea commented Jun 22, 2016

Carreau commented Jun 22, 2016

Choose a reason for hiding this comment

minrk commented Jul 6, 2016

rmorshea commented Feb 1, 2017

rmorshea commented Mar 16, 2017

ankostis commented Mar 16, 2017

Choose a reason for hiding this comment

rmorshea Mar 20, 2017 • edited

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

rmorshea Mar 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmorshea commented Mar 21, 2017

rmorshea commented Apr 19, 2017

rmorshea commented Aug 14, 2017

Carreau commented May 31, 2020

rmorshea commented May 23, 2016 •

edited

rmorshea May 31, 2016 •

edited

rmorshea Jun 22, 2016 •

edited

rmorshea Mar 20, 2017 •

edited

rmorshea Mar 20, 2017 •

edited