Wrap single observer to a separate expression

[kitchoi]

Closes #1076

This PR moves the construction of a single observer to a separate expression object.
Expression remains to be user-facing: it aims at providing a user-friendly interface, e.g. instance methods for composing expressions.

The internal expression objects are hidden from the user, and all implement a common interface called _IExpression. (That interface is rather basic at the moment). Despite the name, Expression is not an instance of _IExpression. An alternative naming suggestion (for either renaming Expression or renaming _IExpression is very welcome).

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

[kitchoi]

Again, nothing does issubclass or isinstance check. The registeration is unused. The registration is only here for code readability and discovery (e.g. it is easier to see what implement the interface).

[mdickinson]

Please can you give some examples of how a user might use Expression?

[kitchoi]

Sure.

A user will typically create an instance of Expression using one of the top-level functions such as trait.
e.g. trait("name") returns an instance of Expression.

The parse function from parsing can create an instance of Expression from a string:
e.g. parse("a.b.c") returns an instance of Expression.

Then they can extend it using the instance methods on Expression. e.g. trait("name").trait("child") returns a new instance of Expression.

With that, I don't expect Expression.__init__ to be called directly by the users because there are more convenient ways. That said, the user can, if they really wanted to, instantiate an empty expression with Expression(), that is fine. They could create one with the internal object Expression(_expression=...), but the naming of the internal variable and docstring hopefully make it clear that such usage is not part of the public interface so breakage is possible.

[mdickinson]

Right, but we're specifically making Expression user-facing. I'm wondering what a user would use it for.

I don't think we actually need the Expression class at all.

[mdickinson]

I think we should be able to get rid of this class entirely.

[kitchoi]

Please can you give some examples of how a user might use Expression?

Ah, you are asking about "using" the expression. I am sorry, I misunderstood your question earlier.
An expression is to be used together with HasTraits.observe, e.g.

@observe(trait("parent").trait("child"))
def updated(self, event):
    pass

Alternatively we could just let trait("name") return an instance of _SingleObserveExpression (renamed it to SingleObserverExpression). Likewise, trait("name").trait("attr") will return an instance of _SeriesExpression (renamed to SeriesExpression). Then all the instance methods, e.g. then, will need to be inherited by SingleObserverExpression and SeriesExpression and so forth. Users reading the API documentation will need to find the method on the type of expressions being created. If we wrap all these in a thin facade, then there is only one class users need to go to in order to find documentation.

[mdickinson]

Alternatively we could just let trait("name") return an instance of _SingleObserveExpression (renamed it to SingleObserverExpression). [...]

Yes, I think this is the way to go.

[kitchoi]

Currently, the user manual can just point to this Expression class and cross-reference its API documentation. If we are to remove this, all references to Expression will refer to an interface, not just docstrings in observe, but user manuals.

While experienced developers can understand interfaces and therefore the Expression would seem redundant, the absence of this facade increases the learning curve for those who are new to Python and/or object oriented programming. Considering the goal is to minimize the cognitive load for these users, would you reconsider keeping this facade for that?

[mdickinson]

I think the documentation concerns are secondary here. We should start with a simple, clean object structure, and then we can figure out how to best document it. Complicating the code to simplify the docs would be counterproductive.

The way I'd do this would be to have an Expression base class (technically an abstract base class, but it doesn't need to be actually defined as inheriting from abc.ABC) containing the common code (e.g., the implementation of the then method), and have the concrete subclasses like SingleObserverExpression inherit from Expression. Do away with the interface entirely: just fold the methods into the Expression abstract base class.

[mdickinson]

In case it's not clear what I mean, I made an example PR in #1094 to demonstrate; it's not a huge step away from the existing code. (Note that I didn't do any renaming: just trying to communicate the shape of the suggested changes, not trying to get all the details right.)

[kitchoi]

No problem. I understand exactly what you meant, and have made the exact same changes you have in #1094 (to the point they are identical). Would you like me to push those changes here from your PR?

[mdickinson]

No, ignore my PR. I'll close it. :-)

[kitchoi]

Drive-by: Without the space Sphinx refuses to display trait as code.

[kitchoi]

No, ignore my PR. I'll close it. :-)

Thank you for diving in though. Much appreciated.

[mdickinson]

Could we drop this branch? Is it required for correctness, or is it more for optimization?

[mdickinson]

(And if it's required for correctness, what happens with something like expr1 | expr2 | expr1?)

[kitchoi]

This is an optimization and yes we can drop it.

[mdickinson]

LVGTM. One outstanding minor question / comment.