Add warning while observing container items with comparison mode set to equality

[kitchoi]

Part of #977

This PR adds a warning when item changes in a list/dict/set are observed, if the list/dict/set is a top-level container and the comparison_mode is set to equality.

Motivation: When a new container is assigned to a trait where the comparison mode is set to equality (the default), no change event will be fired if the new container compared equally to the old one. Consequently, mutation and extended trait change cannot be observed on the new container, and the old container (if it is still around) will continue to emit notifications. This would be an unexpected behaviour.

Changing the default comparison mode is going to break code. So instead, the warning instructs users to fix this by setting comparison_mode to identity, like this:

class Foo(HasTraits):
    container = List(comparison_mode=1)

Note that the warning is not emitted until an observer is requested, this is such that we don't create warnings for existing code using on_trait_change. The downside is that the warning occurs late.

Checklist

• Tests
• Update API reference (docs/source/traits_api_reference) <--- Not sure where this can go
• Update User manual (docs/source/traits_user_manual): Will be in a separate PR
• Update type annotation hints in traits-stubs

[kitchoi]

Confirmed the test suite is still quiet: That is because tests on observers such as ListItemObserver have by-passed observers on trait (e.g. NamedTraitObserver) and operate directly on the container observables:

traits/traits/observers/tests/test_list_item_observer.py

Lines 231 to 239 in 78bc0d9

	graph = create_graph(
	ListItemObserver(notify=True, optional=False),
	)
	handler = mock.Mock()
	call_add_or_remove_notifiers(
	object=instance.custom_trait_list,
	graph=graph,
	handler=handler,
	)

The warning requires an observer observing a CTrait being followed by an observer observing container items.

We will likely need integration tests bringing together different observers, but that will be another PR, and those tests will run into this warning if comparison_mode is not set properly.

[mdickinson]

Code looks okay, modulo a couple of nitpicks, but I'm wondering what the long-term picture looks like here.

If we end up in a place where code has transitioned to using observe, we're going to see a lot of comparison_mode=ComparisonMode.identity noise in our HasTraits classes. Any thoughts about how we might eventually get rid of that noise?

One possible plan is to eventually make comparison_mode=ComparisonMode.identity the default for these trait types; we'd have to do that after most code had transitioned to using observe, and with suitable warnings (probably over several releases).

Another possibility (orthogonal to the first) would be to provide convenience aliases: e.g., IList(...) = List(..., comparison_mode=ComparisonMode.identity). (Thinking "I" for "Identity", but we'd have to figure out the right name.)

[mdickinson]

Please pull this out to a module-level function; I don't think there's any need for it to be a nested function here.

[mdickinson]

We shouldn't rely on ordering being meaningful for ComparisonMode entries. Let's instead check for ComparisonMode.equality directly. If you want to make this code more defensive against possible future additions to ComparisonMode, you could raise NotImplementedError for comparison modes other than the known three.

[kitchoi]

One possible plan is to eventually make comparison_mode=ComparisonMode.identity the default for these trait types; we'd have to do that after most code had transitioned to using observe, and with suitable warnings (probably over several releases).
Another possibility (orthogonal to the first) would be to provide convenience aliases

Yes I think both of these ideas are nice and can work together. In particular, I agree that it would be best to have the default comparison mode set to identity eventually. And I agree that manually adding the comparison_mode=ComparisonMode.identity in the transition period is a hassle, possibly enough to prevent the migration from taking place.

Are you thinking about something like this?:

• In addition to having this warning, provide a way to switch identity comparison mode that is as easy as find-all-and-replace.
  A function that wraps the class might work too, like this: partial(List, comparison_mode=ComparisonMode.identity).
• In some future release, pull comparison_mode out of the keyword arguments of List.__init__ (and friends), with the default None in the signature. If it is None, use ComparisonMode.equality for backward compatibility but put up a deprecation warning. The deprecation warnings are going to be very noisy if code has not done the switch early on.
• In a more future release, switch the default comparison mode to equality identity and remove the deprecation warning.

After all these, I think the warning might still need to stay in case someone really needs the comparison mode to be equality and they need observe: it would be a free choice to live with the limitations. I suspect that situation will be rare though.

Note there is also a scenario where Union is used with a container type as its inner trait, and the identity comparison mode needs to be defined on the Union, not on the container type (see test in this PR). If the default comparison mode on Union remains to be equality, setting the comparison mode to identity on Union will have to be done manually I think. We could also compute the default comparison mode using the inner trait types, i.e. default to identity if any of the trait type is List/Dict/Set.

Shall we merge this PR and continue the discussion in a separate issue?

[kitchoi]

In a more future release, switch the default comparison mode to equality and remove the deprecation warning.

I'm sorry, the "equality" in this sentence should be "identity". Updated the comment.

[mdickinson]

Shall we merge this PR and continue the discussion in a separate issue?

Let's do that.

[mdickinson]

LGTM

[kitchoi]

Opened #1122