Skip to content
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

Observe Traits Via Metadata #111

Closed
wants to merge 13 commits into from
Closed

Conversation

rmorshea
Copy link
Contributor

Allows the use of metadata as a way to observe traits

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

For a more complete demonstration of the new features see test_traitlets.py:

  • test_observe_decorator_via_tags
  • test_observe_via_tags
  • test_unobserve_via_tags

@minrk
Copy link
Member

minrk commented Oct 21, 2015

Needs tests for new functionality.

@ellisonbg
Copy link
Member

I like this idea - it will help with certain patterns.

Question: does the example above mean that the bar field exists in the tags or has a value of True?

@SylvainCorlay
Copy link
Member

👍

@SylvainCorlay
Copy link
Member

It is a problem of argument pass-through. Since the corresponding call to HasTraits::traits in your example is traits(bar=True)... Since observe already takes keyword arguments, we cannot do the same and are forced to pass a dictionary.

@rmorshea
Copy link
Contributor Author

Begs the question of whether 1ae7d72 is reasonable as well.

type : str or All (default: 'change')
The type of notification to filter by. If All, the specified handler
is uninstalled from the list of notifiers corresponding to all types.
"""
names = parse_notifier_name(names)
if tags:
names.extend(self.trait_names(**tags))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this well, if names is All, then you end up listening to the "All" event and the filtered tags. I would rather compute a list of names from tags before calling parse_notifier_name.

@rmorshea rmorshea force-pushed the observe_tags branch 2 times, most recently from d654654 to 48679e4 Compare October 22, 2015 03:02
@rmorshea
Copy link
Contributor Author

48679e4 is a pretty drastic change, but it solves the problem of associating arbitrary notifiers with tags. Given that, I need to think on it to be sure it isn't fundamentally flawed, but feedback would be appreciated @SylvainCorlay, @minrk, @ellisonbg.

The gist of it:

  • All notifiers are wrapped in EvenHandlers
  • The _trait_notifiers and _trait_validators HasTraits attributes are lists instead of dicts.
  • Collecting callbacks is done using the names and tags attrs on EventHandlers and requires this to be computed whenever _notify_change is called (this is because lists are used instead of dicts).

I have a feeling the _trait_notifiers and _trait_validators lists might slow down how quickly notifiers are called since collecting them isn't done via dictionary keys. If that's the case, dicts can be used (it just seemed redundant to have the name and type data in both the dict and on the EventHandler). With dicts though, having a helper function that flattens _trait_notifiers into a list might be convenient since the information can be present both ways.

@SylvainCorlay
Copy link
Member

This is quite a big and non-trivial change. Can we work on pushing 4.1/5.0 first and get this in later? I am 👍 on the feature.

@ellisonbg
Copy link
Member

+1 on waiting then

On Thu, Oct 22, 2015 at 10:44 AM, Sylvain Corlay notifications@github.com
wrote:

This is quite a big and non-trivial change. Can we work on pushing 4.1/5.0
first and get this in later? I am [image: 👍] on the feature.


Reply to this email directly or view it on GitHub
#111 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@rmorshea rmorshea force-pushed the observe_tags branch 3 times, most recently from 8156a35 to 3386ca2 Compare October 24, 2015 09:52
@rmorshea
Copy link
Contributor Author

I've cleaned this up significantly and added a heuristic for triggering change types other than "change".

But yes, probably best to wait.

@rmorshea rmorshea force-pushed the observe_tags branch 3 times, most recently from f2be7c0 to 2383466 Compare December 26, 2015 17:50
@minrk minrk modified the milestones: 4.2, 4.3 Mar 14, 2016
@rmorshea rmorshea force-pushed the observe_tags branch 4 times, most recently from 1559adb to 45ad032 Compare April 1, 2016 22:38
@rmorshea
Copy link
Contributor Author

rmorshea commented Apr 3, 2016

@minrk, @SylvainCorlay I think I've brought this to a point where it might be worth doing some review.

These changes are quite drastic in order to incorporate all the features that tags can (but perhaps shouldn't necessarily) introduce to observing to traits. To hide away some of the nastier heuristics and keep the main file as simple as possible, I created traitlets/utils/dict_types.py

For a more complete demonstration of the new features see test_traitlets.py:

  • test_observe_decorator_via_tags
  • test_observe_via_tags
  • test_unobserve_via_tags

At this point, everything is included and functional, so we can trim features as we see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants