Add user-facing Expression for composing a graph of observers

[kitchoi]

Closes #1024
Item 4 of #977

This PR adds the Expression class which allows user to compose a graph of items to be observed. No specific observers are included here, so the Expression at the moment isn't very useful. A proof-of-concept of this was previously implemented in #969.

• Implemented __or__ for combining two expressions to share the same parent, needed for "a, b"
• Implemented then for combining two expressions so that the latter expression is the child of the former one, needed for "a.b" in the mini-language
• Implemented __eq__ for comparing two expressions
  - In fact, this is merely for the convenience of the users and for testing. For the user, one can do things like parse("a,b") == parse("b, a") and this helps learning the expression logic. For tests, the equality is useful for testing the mini-language parser later.

After this PR, we can easily add the wrapper for each specific observer independently and the Expression class will become more useful.
e.g. like this:

traits/poc/expressions.py

Lines 157 to 180 in b8991fb

	def trait(self, name, notify=True, optional=False):
	""" Create a new expression that matches the current
	expression and then a trait with the exact name given.
	e.g. ``trait("child").trait("age")`` matches ``child.age``
	on an object, and is equivalent to
	``trait("child").then(trait("age"))``
	Parameters
	----------
	name : str
	Name of the trait to match.
	notify : boolean, optional
	Whether to notify for changes.
	optional : boolean, optional
	Whether this trait is optional on an object.
	Returns
	-------
	new_expression : Expression
	"""
	return self._new_with_branches(
	nodes=[NamedTraitListener(name, notify, optional)],
	)

Checklist

• Tests
• Update API reference (docs/source/traits_api_reference)
• Update User manual (docs/source/traits_user_manual)
• Update type annotation hints in traits-stubs: We can have annotation hints for the high-level functions like trait and metadata. This PR does not introduce those yet.

[codecov-io]

Codecov Report

Merging #1067 into master will decrease coverage by 2.95%.
The diff coverage is 62.55%.

@@            Coverage Diff             @@
##           master    #1067      +/-   ##
==========================================
- Coverage   76.15%   73.20%   -2.96%     
==========================================
  Files          54       67      +13     
  Lines        6493     8159    +1666     
  Branches     1263     1558     +295     
==========================================
+ Hits         4945     5973    +1028     
- Misses       1205     1807     +602     
- Partials      343      379      +36     

Impacted Files	Coverage Δ
traits/api.py	100.00% <ø> (+9.67%)	⬆️
traits/base_trait_handler.py	61.76% <ø> (ø)
traits/ctrait.py	71.07% <ø> (ø)
traits/has_traits.py	72.40% <ø> (-0.37%)	⬇️
traits/observers/_i_notifier.py	0.00% <0.00%> (ø)
traits/observers/_i_observable.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%> (ø)
... and 35 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...51948de. Read the comment docs.

[corranwebster]

This looks good to me.

[mdickinson]

Could we tweak/augment the API so that it's possible to create an Expression in a declarative manner? Right now it seems as though the only way to create an Expression object is to create an empty expression and then mutate it; it would be great to be able to instead create an Expression directly. I see that the tests use _new_with_branches. Could we have something similar that's public?

[kitchoi]

@mdickinson Are these what you are looking for?:

traits/traits/observers/expressions.py

Lines 541 to 551 in 59d8769

	trait = _as_top_level(Expression.trait)
	list_items = _as_top_level(Expression.list_items)
	dict_items = _as_top_level(Expression.dict_items)
	set_items = _as_top_level(Expression.set_items)
	metadata = _as_top_level(Expression.metadata)
	filter_ = _as_top_level(Expression.filter_)

Here is an example of the usage:

traits/traits/observers/tests/test_expressions.py

Line 214 in 59d8769

expr = expressions.trait("name").trait("age")

(expressions is the module here).

[mdickinson]

@kitchoi Thanks, but not really. It's the awkwardness of creating an Expression that's bothering me.

I'm finding it hard to wrap my head around what an Expression is here, and the class feels awkward and complicated to work with; I know what I expect it to be, which is a tree-structure matching the expression used to create it (whether that's a Python-level DSL-type expression, or a string). For example, the fact that in the tests you need to do Expression()._new_with_branches(nodes=[observer]) to create a simple expression with a single observer seems like a smell. I'd expect to see something as simple as ObserverExpression(observer). More generally, I'd expect to be able to create these Expressions in a declarative manner, much like we build up a TraitsUI View.

I think what I expect isn't a world away from what you have; it may be mostly a matter of naming.

I think what we want is three top-level classes OrExpression, ThenExpression and ObserverExpression, possibly all inheriting from an Expression abstract base class (or implementing an IExpression interface); OrExpression and ThenExpression would each have a left and right child (which would be arbitrary Expression instances), and it should be possible to create an expression direct using class constructors, using something like OrExpression(ObserverExpression(obs1), ObserverExpression(obs2)).

I think your _prior_expression is acting like the left child of an ObserverExpression or ThenExpression. Is that right?

I'd also expect these expression objects to be immutable; it looks as though they effectively are already in this PR: the only point at which they're being mutated is immediately after creation. We could remove the need to mutate entirely by adding suitable initialization logic to the __init__.

And if they're immutable, then we don't have any need to copy things. (I'm not quite clear on why there are copies going on at the moment.)

Take a look at the structure of Python's AST module (maybe just the expression piece) for inspiration; I think that's the sort of structure that we want here, though obviously it'll be much much simpler.

[mdickinson]

IOW, what I think we want here is a Python-level representation of the following abstract data type (see e.g. https://en.wikipedia.org/wiki/Algebraic_data_type):

    Expression = OrExpression(Expression, Expression)
               | ThenExpression(Expression, Expression)
               | ObserverExpression(IObserver)

Python unfortunately doesn't have GADTs or unions or generalised Enum types like other languages, so the natural "Pythonic" way to represent the above datatype is using three separate classes OrExpression, ThenExpression and ObserverExpression.

[mdickinson]

Do we need this extra complication? I think I'd expect equality for an Expression simply to be structural equality.

[kitchoi]

This is merely for the users. As a user, I would expect trait("a") | trait("b") and trait("b") | trait("a") to be equivalent. It also does not hurt to make them not equal though. Nothing else depend on this.

[kitchoi]

Overlooked this :(. See #1080

[mdickinson]

Naming nitpick: I suggest expression rather than expressions for the module name.

[kitchoi]

@mdickinson Thank you for the review and suggestions.

The _SeriesExpression and _ParallelExpression are essentially the ThenExpression and OrExpression you have there.

Maybe we will need this ObserverExpression(IObserver), but at the moment the test utility function is the only such usage, and it is only there because I don't have any actual observer to work with at the moment (when I have all the observers, I can remove this test helper function calling out to _new_with_branches). If we do have this ObserverExpression(IObserver) layer, it won't be exposed to the user either as I am making a deliberate effort to hide IObserver from the users, so could we do that later?

[mdickinson]

The _SeriesExpression and _ParallelExpression are essentially the ThenExpression and OrExpression you have there.

So ignoring naming, the difference is that I'd like __or__ on an Expression to return a _ParallelExpression directly, instead of needing an extra level of indirection through _prior_expression; and similarly for then. I think we can then get rid of Expression._prior_expression altogether and also remove any copying.

I'm suggesting an ObserverExpression to encapsulate the _levels logic that you already have; maybe ObserverExpression is not the right name here. But whatever makes sense for the use-case which an Expression with _level but no _prior_expression is targeted at.

So again modulo naming, I think we want to restructure into 4 classes: the current Expression, _SeriesExpression and _ParallelExpression all living at the same level, and all inheriting from a base class (AbstractExpression). The implementations of then and __or__ would live on that base class.

I think we can make the structure and logic more obvious here, which would help future maintainers working with this code.

[mdickinson]

@kitchoi Please open an issue for addressing the refactoring of the Expression class and add it to the 6.1 milestone.

[kitchoi]

For the record, I gave the restructuring a go on my test branch:
5657dd4
And all tests, including the final integration tests (apart from the ones removed) pass without any modifications.

However this test branch does not have recursion feature that we have decided to delay. In other words, I have not tested the new structure with recursion support. From previous experience, recursion support has a fundamental influence on the data structures.

I am confident that any of these restructuring will not affect public facing API.

[kitchoi]

I will however make copy private, it should not be used by the end users.

[kitchoi]

Merging now so that we can proceed with the next step.