Add observe decorator and instance method #1086

kitchoi · 2020-05-15T11:35:14Z

This PR implements item 14 of #977

Expose the observe decorator and HasTraits.observe instance method.

Details:

Add observe function in the observers subpackage (to be renamed to observe too)
- This prevents HasTraits from knowing any more internal details than necessary.
- Maybe for discussion: This also allows advanced users to supply their own dispatcher function, e.g. gevent.spawn without requiring another release of Traits, or requiring registration mechanism
Add HasTraits.observe method in the traits API. It is similar to HasTraits.on_trait_change.
- Naming: name is renamed to expression.
- The overloaded signature: A list is supported, so that people migrating from on_trait_change don't have to combine a list of string (preferred by me) into a comma-separated string. This will be more compatible with the future counterpart of Property depends_on.
- The overloaded signature: All strings are parsed to Expression object.
- Previously name is optional and can be None to mean anytrait. Now expression is a required positional argument and cannot be None. The use case of anytrait can be supported by the filter_ expression (to be introduced in a future PR, related to Add an observer for observing traits using an arbitrary filter #1078).
- The positional argument order of (handler, expression) is retained to be consistent with on_trait_change. This will lower the barrier for migrating to observe.
Add @observe decorator in the traits API. It is very similar to @on_trait_change in that it wraps the HasTraits.observe method and it supports post_init flag.
Added traits.observers.api and expanded traits.api

Checklist

Tests
Update API reference (docs/source/traits_api_reference)
~~Update User manual (docs/source/traits_user_manual)~~ : Will be in a separate PR
Update type annotation hints in traits-stubs

mdickinson

Done with first-pass review. Could you explain a bit more how the observers dict is working in update_traits_class_dict? It's not clear to me why it makes sense to put both trait names and method names in there. (I'm not saying it doesn't make sense; just that I'm not yet seeing how it works.)

mdickinson · 2020-05-18T08:27:17Z

traits/ctraits.c

@@ -735,6 +735,14 @@ has_traits_init(PyObject *obj, PyObject *args, PyObject *kwds)
        Py_DECREF(value);
    }

+    /* Make sure all of the object's observers have been set up: */
+    value = PyObject_CallMethod(obj, "_init_trait_observers", "()");


It would be better to use NULL instead of "()" for the third argument: that's the documented way to spell "no arguments", and it avoids doing runtime parsing of the format string.

We should change the pre-existing cases of a "()" format too, but not in this PR. If you prefer to defer all the changes to a separate PR, that's fine, too.

mdickinson · 2020-05-18T08:45:27Z

traits/ctraits.c

+    if (value == NULL) {
+        return -1;
+    }
+


Spacing nitpick: suggest removing this blank line. (I know this spacing matches what's elsewhere in this function, but to me the Py_DECREF is part of the same conceptual code block as the call and error check.)

But this is totally a nitpick: feel free to ignore.

mdickinson · 2020-05-18T08:54:52Z

traits/has_traits.py

@@ -385,6 +394,11 @@ def update_traits_class_dict(class_name, bases, class_dict):
    prefix_list = []
    view_elements = {}

+    # Mapping from method name or trait name to list(dict)


Can you elaborate on how method names are being used here? The description here is surprising at first glance (that the keys of the dictionary could be either a method name or a trait name). How does this work, and why don't we need two separate dictionaries?

I am sorry for the confusion - the keys here are only method names.
I went ahead with the Property support in the other branches where the name (defined in the class dict) could refer to the name of a trait that is a property. Here we are not dealing with property so that half of the comment is misleading. I will remove that.

mdickinson · 2020-05-18T09:52:57Z

traits/has_traits.py

+        value       dispatch
+        =========== =======================================================
+        ``same``    Run notifications on the same thread where the change
+                    occurs.


For ease of comparison with the "ui" case, it may also be worth documenting here that the listeners/observers are executed immediately in this case.

mdickinson · 2020-05-18T09:54:03Z

traits/has_traits.py

+    HasTraits.observe
+    """
+
+    def decorator(function):


decorator is a generic name here; can we use something more informative? These names could potentially show up in tracebacks or during debugging. (Same for function.)

mdickinson · 2020-05-18T09:56:39Z

traits/has_traits.py

+        Parameters
+        ----------
+        function : callable
+            Unbound method of a subclass of HasTraits


"Unbound method" isn't quite right here: Python 3 doesn't have unbound methods any more. What's being passed here is simply a function. Not sure what to suggest instead, though: maybe just drop the word "Unbound" and say "Method of a HasTraits subclass"?

mdickinson · 2020-05-18T09:57:23Z

traits/has_traits.py

+        """ Create input arguments for HasTraits.observe and attach the input
+        to the callable.
+
+        Metaclass will then collect this information for calling


Suggested change

Metaclass will then collect this information for calling

The metaclass will then collect this information for calling

mdickinson · 2020-05-18T10:00:12Z

traits/has_traits.py

@@ -2066,6 +2260,8 @@ def on_trait_change(
        """Causes the object to invoke a handler whenever a trait attribute
        matching a specified pattern is modified, or removes the association.

+        See also ``HasTraits.observe`` for a newer API.


Could use the Numpy doc "See Also" header here to make this more prominent.

mdickinson

LGTM. One docstring suggestion.

kitchoi · 2020-05-19T07:56:49Z

Thank you.

One docstring suggestion.

I think I lost it... maybe GH buried it, could you point me to it please?

mdickinson · 2020-05-19T08:02:30Z

I think I lost it

I've lost it, too. I frequently add a comment, forget to hit the "submit" button on that comment, and then navigate away; this might have been one of those cases. Sorry.

LGTM

traits-stubs/traits-stubs/api.pyi

kitchoi · 2020-05-19T08:08:01Z

No problem. Thanks.

kitchoi added 19 commits May 14, 2020 15:51

Add a test for the purpose of target

071eafd

Merge branch 'master' into 977-observe-top-level

44d5a65

Add top-level observe function

885f5b0

Expose the api module for observe

fb7372c

Add more top-level functions to observers api.py

4a6efc7

Implement observe decorator and instance method

1e09331

Handle overloaded signature differently

e661eb9

Improve docstring

fb479a9

Improve comment

1d60403

Fix another comment

5370e82

More comment fixes

38787be

Update type annotation hints

956e2c7

Update test on dispatch with UI

d565b94

Remove redundancy in tests

7e83fb0

Simplify tests even more

6f309c4

Flake8

1bd9122

Actually use list, because on_trait_change does it too

f32c0b8

Hold back on exposing the exception handlers for now

aac99bb

Import abcdef...

14dd7d0

kitchoi mentioned this pull request May 15, 2020

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

Closed

19 tasks

mdickinson self-requested a review May 18, 2020 08:12

mdickinson requested changes May 18, 2020

View reviewed changes

kitchoi added 8 commits May 18, 2020 16:03

Merge branch 'master' into 977-observe-top-level

3897709

Fix misleading comment

9117602

Use NULL when call a method with no arguments

653a360

mention the notifications are run immediately for ease of comparison

197a462

Rename decorator -> observe_decorator, function -> handler

e2678e2

Docstring: Metaclass -> The metaclass

6b30d01

Use See Also section for mentioning observe

f3b4e18

See also -> See Also

7fd7bda

kitchoi added 2 commits May 18, 2020 17:27

Fix grammar

45a00a7

Add See-Also for on_trait_change decorator too

17c56da

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

mdickinson mentioned this pull request May 19, 2020

Fix uses of "()" format string in PyObject_CallMethod #1099

Closed

mdickinson approved these changes May 19, 2020

View reviewed changes

mdickinson mentioned this pull request May 19, 2020

Use NULL for zero-argument PyObject_CallMethod format #1100

Merged

mdickinson reviewed May 19, 2020

View reviewed changes

traits-stubs/traits-stubs/api.pyi Show resolved Hide resolved

kitchoi merged commit 1fe353a into master May 19, 2020

kitchoi deleted the 977-observe-top-level branch May 19, 2020 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add observe decorator and instance method #1086

Add observe decorator and instance method #1086

kitchoi commented May 15, 2020 •

edited

Loading

mdickinson left a comment •

edited

Loading

mdickinson May 18, 2020

mdickinson May 18, 2020

mdickinson May 18, 2020

kitchoi May 18, 2020

mdickinson May 18, 2020

mdickinson May 18, 2020

mdickinson May 18, 2020

mdickinson May 18, 2020

mdickinson May 18, 2020

mdickinson left a comment

kitchoi commented May 19, 2020

mdickinson commented May 19, 2020

kitchoi commented May 19, 2020

	Metaclass will then collect this information for calling
	The metaclass will then collect this information for calling

Add observe decorator and instance method #1086

Add observe decorator and instance method #1086

Conversation

kitchoi commented May 15, 2020 • edited Loading

mdickinson left a comment • edited Loading

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

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

kitchoi commented May 19, 2020

mdickinson commented May 19, 2020

kitchoi commented May 19, 2020

kitchoi commented May 15, 2020 •

edited

Loading

mdickinson left a comment •

edited

Loading