Implement ListItemObserver for observing mutations to a list #1071

kitchoi · 2020-05-08T11:06:34Z

This PR implements item 8 of #977:

Implement the logic for attaching notifiers when the items of a list changes

Add ListChangeEvent for representing mutations to a list. This object will be given to the user's change handler.
ListItemObserver implements the interface of IObserver to:
- return the object for adding/removing notifiers. In this case, the observable is simply an instance of TraitList. If the incoming object is not a TraitList and the observer is not marked as optional, the observer will complain. This is to help catch usage errors.
- yield the content of the list for the next observer(s) in an ObserverGraph. The ListItemObserver has no knowledge about the nature of the next observers (this is generally true for any observers). If the content of the list is not compatible with the next observer (say the list contains integers), the next observer is expected to complain about it. e.g. see NamedTraitObserver in Implement the logic for observing a named trait #1069 which raises an error when the incoming object is not an instance of HasTraits.
Implement IObservable interface in TraitList. With this, any subclass of TraitList, e.g. TraitListObject, can be observed with ListItemObserver (see tests).

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~~

…Observer

ievacerny

Found a few small things, otherwise LGTM! Although someone else should review this as well

ievacerny · 2020-05-08T18:23:52Z

traits/observers/_list_change_event.py

+    Attributes
+    ----------
+    trait_list : list
+        The list being mutated.


I think it would be good to clarify whether this is a list before or after changes (same in event factory).

Whether the list contains content before or after the change depends on whether the change event is fired before or after the mutation, because it is the same list object. Strictly speaking, the event object does not need to dictate when the notification should be fired. Not sure how I could write this...

We could add to the API document (not here) that notifications are fired after the mutations. What do you think?
i.e. TraitList.notify are called after the mutation has taken place.

Oh, okay, I thought the behaviour was consistent. I think it would be good to report which events are fired before changes and which are fired after, but I agree that the event itself is not a place for that.

I am sorry I did not mean to imply there were events fired before changes. All events are fired after changes. Sorry for the confusion there.

Okay, well I would still argue that it would be good to inform the user that the event is fired after changes are made, but then I really don't know what would be the right place to do so.

Yes I agree that the information needs to be somewhere in the documentation. In fact, I would very much like that to be included in the user manual.

While it may be convenient to have that description in the event object parameter section, doing so may introduce restrictions that do not belong to the data structure.

Actually, the first sentence in the current user manual already implies that all change notifications are fired after the change:

https://docs.enthought.com/traits/traits_user_manual/notification.html

I see that similar implicit mentions are made in the new observer's user manual as well, so it might be enough. I was just a bit worried that because the handlers are now supposed to deal with the actual events, the user's might be paying more attention to the docstrings of those events.

It might not be amplified enough, but it is a fairly strong feature of Traits that notifications always fire after the value has set.

traits/observers/_list_item_observer.py

traits/observers/tests/test_list_item_observer.py

codecov-io · 2020-05-11T09:49:06Z

Codecov Report

Merging #1071 into master will decrease coverage by 3.12%.
The diff coverage is 61.30%.

@@            Coverage Diff             @@
##           master    #1071      +/-   ##
==========================================
- Coverage   76.15%   73.03%   -3.13%     
==========================================
  Files          54       67      +13     
  Lines        6493     8103    +1610     
  Branches     1263     1548     +285     
==========================================
+ Hits         4945     5918     +973     
- Misses       1205     1806     +601     
- 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/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%> (ø)`
traits/trait_types.py	`72.15% <80.00%> (-0.31%)`	⬇️
... and 34 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 dfc6ffa...eaf7573. Read the comment docs.

kitchoi · 2020-05-11T09:56:09Z

Thank you @ievacerny for the review!
I hope I have addressed all the comments.
There will be some trivial merge conflicts with #1069 due to the shared API module and documentation source. I will resolve those when that happens.

traits/observers/_list_change_event.py

corranwebster · 2020-05-11T16:25:33Z

traits/observers/_list_change_event.py

+        )
+
+
+def list_event_factory(trait_list, index, removed, added):


I'd be tempted to make this a classmethod of ListChangeEvent

Actually, on second thought, perhaps embrace the ordering and make ListChangeEvent a named tuple.

The __init__ of ListChangeEvent is currently the same as the call signature of TraitList.notify, but they could evolve independently of each other. DictChangeEvent in #1072 is actually an example where the __init__ of DictChangeEvent is not the same as the call signature of TraitDict.notify.

The only reason why this is not a classmethod is to avoid exposing this to the user. ListChangeEvent is exposed in the public API via the public module events whereas the event factory isn't. An alternative organization would be to have all the public facing event object defined in a public module, and these event factories defined in another private modules.

The init of ListChangeEvent is currently the same as the call signature of TraitList.notify

That's not true because of the keyword arguments...and of course that's why you asked in the next comment 😅

corranwebster · 2020-05-11T16:29:59Z

traits/observers/_list_item_observer.py

+
+
+@IObserver.register
+class ListItemObserver:


As in the last PR, can this be a subclass of named tuple?

Also had the thought that even if you don't turn it into a named tuple, using __slots__ here might make sense for efficiency (keep that in the back of your mind, even if it isn't part of this PR).

I did give this a go, and while it got rid of the __init__, __hash__, __eq__ and the properties, the equality check does not quite do what is implemented here. e.g. the following returns true.

ListItemObserver(notify=True, optional=False) == (True, False)

That is not very convenient considering that ListItemObserver, SetItemObserver and DictItemObserver have the same attributes and they should not be equal to each other.

Let's keep __slots__ away from this for now; using __slots__ can have some interesting side-effects (particularly with regard to subclassing). It's an optimization that we should save until we need it (and my bet would be that we won't ever need it).

corranwebster · 2020-05-11T16:30:28Z

traits/observers/_list_change_event.py

+# ListChangeEvent is exposed in the public API
+
+
+class ListChangeEvent:


Possibly make this a named tuple?

namedtuple turns this into a sequence-like object. Unlike the observer, this is a public-facing object, so it would be difficult going back. Maybe better to wait until we really need to.

I agree about avoiding namedtuple; it's an interface that presents multiple ways to do it, which often isn't what you want. It's useful as a transitional type when going from something that was previously a tuple to something closer to a class with attributes, but I don't think it's what we want here. A dataclass may be closer to what we want, but that's not available until Python 3.7.

corranwebster · 2020-05-11T16:33:36Z

traits/observers/_list_change_event.py

+        Values removed from the list.
+    """
+
+    def __init__(self, *, trait_list, index, removed, added):


Is there are particular reason you want these to be keyword only?

Enforcing keyword-only arguments will help prevent bugs like this one: enthought/traitsui#791

I agree that keyword-only arguments make sense here: it makes the calling code clearer as well, particularly with respect to the ordering of removed and added - someone reading the client code doesn't have to check or remember which way around the arguments go.

The keyword-only restriction is also something that's easy to relax if it becomes problematic. It's harder to add the restriction back in if it's not present to begin with.

corranwebster · 2020-05-11T16:57:27Z

traits/observers/_list_change_event.py

+
+    Attributes
+    ----------
+    trait_list : list


Also, a design suggestion for this and the other collection change events: it might make sense to try to mimic the API of TraitChangeEvent. At a minimum:

using object instead of trait_list

a dummy attribute name which is always "items"

This might allow code to consume both types of event without having to do a type check if it only cares about the object and the name.

That would imply an implicit "ChangeEvent" interface where you expect both "object" and "name" to exist. It is tempting to make new be the new value of the list (ie. the same as object) and old to be a property that lazily reconstructs the old value of the list from the added and removed, but that isn't really right from a philosophical point of view.

Really the rest of the information is the "delta", ie. we have

"who changed": the object that was changed, be it a HasTraits, TraitList, etc.

"what changed": the name of the thing that changed (the thing that was being observed)

"how it changed": the nature of the changes that took place (old/new, added/removed, etc.)

Thinking this way may help with the design of these classes.

Hmmm we might need some use cases to drive this. For example, if a handler needs to consume object and name, the next thing it does may be to call getattr(object, name), and doing that with object being TraitList and name being "items" is going to fail.

If the handler needs to do something specific about the change details, then it probably needs to know which type of event objects it gets anyway, it could go two routes:
(1) Apply notify=True only on the type of change that is compatible. Then it only deals with one type of event objects.
(2) Check the event type, and do different things according to the type.

Are there use cases already we can draw conclusions from? Otherwise, leaving these object and name undefined for now might actually be more flexible, so we don't have to commit to a value before knowing how they are going to be used. e.g. object could be the HasTraits instance that contains the list somewhere (note that the list could be nested). It does not hurt if we end up having both object and trait_list pointing to the same thing.

"how it changed": the nature of the changes that took place (old/new, added/removed, etc.)

This is one of the reasons that I think it would make sense to bundle index, removed and added up into a single object, as I've suggested before. Those three pieces of information are always generated together, passed around together and used together.

Maybe open an issue to revisit this once the various related PRs are merged and we have a chance to look at the design as a whole?

I am open to that discussion and it makes sense to do that at the very end instead of blocking these PRs.
Do you mind opening the said issue please (since I am worried I may misunderstand the scope you are thinking about)?

Do you mind opening the said issue please

Opened #1084.

kitchoi · 2020-05-13T11:05:52Z

I have addressed the ones that I could address. There are some pending discussion points and I am not sure what to do with those.

mdickinson

A few minor comments and change requests.

traits/observers/_list_change_event.py

traits/observers/_list_item_observer.py

traits/observers/tests/test_list_change_event.py

traits/observers/_list_change_event.py

traits/observers/tests/test_list_item_observer.py

…ey should not be mutated, it is very unlikely they will be mutated.

kitchoi

Thank you for the review! I hope I have addressed all the comments now.

mdickinson

LGTM

@ievacerny and @corranwebster: you both still have outstanding comments against this PR. Is this okay to merge?

mdickinson · 2020-05-15T08:09:58Z

@corranwebster gave his okay on another channel. Merging.

kitchoi added 10 commits May 8, 2020 10:26

Add ListChangeEvent

09d9607

Implement ListItemObserver

f87141c

Update module docstring

d4f8208

Minor change to a sentence

e61d31e

Expose ListChangeEvent to the API doc and a public module

a2fcd75

Compress line changes

c4234ea

Minor fixes to docstring and comments

f35c8ad

Make error message consistent

7c16e30

Flake8

1fb400a

Allow TraitList and not just TraitListObject to be used with ListItem…

02c6366

…Observer

kitchoi mentioned this pull request May 8, 2020

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

Closed

19 tasks

ievacerny reviewed May 11, 2020

View reviewed changes

kitchoi added 4 commits May 11, 2020 10:12

Add optional to imposter

b5d4aa5

Include optional in hash

993d187

Fix tense for removed and added

6b7c728

Merge branch 'master' into 977-list-item-observer

eaf7573

corranwebster reviewed May 11, 2020

View reviewed changes

traits/observers/_list_change_event.py Outdated Show resolved Hide resolved

corranwebster reviewed May 11, 2020

View reviewed changes

kitchoi added 2 commits May 12, 2020 13:36

Merge branch 'master' into 977-list-item-observer

3b6d2e6

Programmatically get the class name in repr

eae92f1

kitchoi requested a review from corranwebster May 13, 2020 11:06

mdickinson self-requested a review May 13, 2020 12:33

mdickinson requested changes May 13, 2020

View reviewed changes

kitchoi added 6 commits May 13, 2020 14:40

Use class name for hash

6d462df

Use TraitList for the type of TraitChangeEvent.trait_list

49e739c

Remove <> in repr

f2de7b5

Use string formating

263538d

Make notify and optional mutable to avoid boilerplate, even though th…

966b477

…ey should not be mutated, it is very unlikely they will be mutated.

Import order

3775641

kitchoi commented May 13, 2020

View reviewed changes

kitchoi requested a review from mdickinson May 13, 2020 14:24

kitchoi mentioned this pull request May 13, 2020

Relax immutability in NamedTraitObserver for consistency #1082

Merged

1 task

mdickinson mentioned this pull request May 14, 2020

Unify APIs for TraitChangeEvent and ListChangeEvent? #1084

Closed

mdickinson approved these changes May 14, 2020

View reviewed changes

kitchoi mentioned this pull request May 14, 2020

Implement SetItemObserver for observing mutations on a set #1075

Merged

2 tasks

corranwebster approved these changes May 15, 2020

View reviewed changes

mdickinson merged commit fd87e9e into master May 15, 2020

mdickinson deleted the 977-list-item-observer branch May 15, 2020 08:10

		# ListChangeEvent is exposed in the public API


		class ListChangeEvent:

Implement ListItemObserver for observing mutations to a list #1071

Implement ListItemObserver for observing mutations to a list #1071

Conversation

kitchoi commented May 8, 2020 • edited Loading

ievacerny left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 11, 2020 • edited Loading

Codecov Report

kitchoi commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitchoi May 11, 2020 • 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

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

kitchoi commented May 13, 2020

mdickinson left a comment

Choose a reason for hiding this comment

kitchoi left a comment

Choose a reason for hiding this comment

mdickinson left a comment

Choose a reason for hiding this comment

mdickinson commented May 15, 2020

kitchoi commented May 8, 2020 •

edited

Loading

codecov-io commented May 11, 2020 •

edited

Loading

kitchoi May 11, 2020 •

edited

Loading