Refactor TraitList / TraitListObject #989
Conversation
Refactor the relationship between TraitList and TraitListObject: - TraitList now only knows about validating individual items, instead of validating the entire list. - Min/max length-checking has moved into TraitListObject. - TraitList operations always call the corresponding list operation, to maintain exception wording.
mdickinson
added
type: refactor
topic: traits listener rework
Codecov Report
@@ Coverage Diff @@
## master #989 +/- ##
==========================================
+ Coverage 73.05% 74.10% +1.04%
==========================================
Files 51 51
Lines 6514 6453 -61
Branches 1309 1277 -32
==========================================
+ Hits 4759 4782 +23
+ Misses 1363 1305 -58
+ Partials 392 366 -26
Continue to review full report at Codecov.
|
For __iadd__ and extend, we need to make sure that we validate _all_ incoming items before we append any of them, else we risk both modifying the list _and_ raising. So replace the corresponding map() calls with list(map()), and convert to a list comprehension for readability. Also converts one other occurrence of list(map()) to a list comprehension, with no change in semantics.
|
From discussion on another channel, this is ready for review and I am assigning myself (unless someone asks me wait). |
|
@kitchoi Thanks for the thorough review. I think I've addressed all comments. |
|
I've also just removed an XXX comment about documenting notification contents. I'm still not sure what to do about that, but I'll open an issue so it doesn't get forgotten. EDIT: see #997 |
|
@midhun-pm Is it okay to merge this, or would like to review before it gets merged? |
|
@mdickinson I haven't done a thorough review, but am referring to it as I make corresponding changes to |
| item_validator : callable, optional | ||
| Called to validate and/or transform items added to the list. The | ||
| callable should accept a single item from the list and return | ||
| the transformed item, raising TraitError for invalid items. If |
There was a problem hiding this comment.
Is it necessary to ensure that the user defined validator raises a TraitError and not something else ? Do we need code to transform the error type if it doesn't?
There was a problem hiding this comment.
What would code to transform the error type look like? We definitely don't want to catch all exceptions here, for the usual reasons (silencing genuine errors, making debugging harder). So would you target particular exception types? If so, which ones, and why?
For this sort of API, where you're delegating responsibility for validation to user code, you want to give that user code a well-defined way to say "nope, this item isn't valid", so that the "invalid" message can be safely distinguished from other messages, like coding errors.
And one clean way to do that is to provide a dedicated exception to use for that; in this case, TraitError.
| @@ -212,471 +156,290 @@ def notify(self, index, removed, added): | |||
| for notifier in self.notifiers: | |||
| notifier(self, index, removed, added) | |||
There was a problem hiding this comment.
Should we catch errors raised by the user defined notifier so that the operation doesn't fail even if the notifier errors out ? There are cases where we return a value after calling notify and this would fail if any notification fails.
There was a problem hiding this comment.
This is a good question, though I think it's bigger than just this one PR. It's possible that we want to catch all failed user notifiers and turn the failures into logged warnings. But that's dangerous: it risks masking coding errors, and we also have internal notifiers to worry about (in fact, that's most of the use-case right now).
So yes, right now we should get a loud failure (an exception and traceback) if any notification fails, and right now that's what we want, since a failed notification likely represents a coding error. (We may still want to be catching exceptions inside notifiers at the point where they call into user callbacks; I think @kitchoi 's work still does that.)
There was a problem hiding this comment.
Indeed, an imminent PR for EEP 3 is going to do the exception handling inside the notifiers.
In addition to the reasons Mark mentioned, this is consistent with how ctraits call_notifiers work (so the notifiers will have to do their error handling anyway). I did not appreciate this earlier but I do now: This allows fine grained control on error handling later in the notifiers. The notifiers do a pile of things before it calls the user-provided handler. We'd want to do a catch-all around the user's handler that is outside traits control, but not around code that belongs to traits.
|
Merging; post-review comments still welcome. |
Here's the promised
TraitList/TraitListObjectrefactor that I suggested last week. The main change is that whole-list validation is gone fromTraitList: instead of the previousvalidatorargument,TraitListnow takes anitem_validatorargument, and that argument is used to validate any items being added in a list operation.In meetings last week we discussed the possibility of a subclass of
TraitListimplementing the length-constraint logic. It turned out to be simplest to merge that proposed subclass withTraitListObject. So this PR doesn't introduce any new classes: we still haveTraitListandTraitListObject, but length validation has moved intoTraitListObject.Rationale for the change:
validatorAPI will extend naturally to future use-cases (for example, maintaining whole-list order); this feels like a case of premature abstraction. The only common use-case we have is item validation. The uncommon use-case we have is compatibility withminlenandmaxlensupport for theListtrait type.added, and the current API doesn't support this.validatorconsistently: for example, it's not called for thesortandreversemethods, and without concrete use-cases it's hard to see what it should do in those cases.TraitListbecomes simpler and clearer if we only need to validate new items.TraitSetandTraitDict, which don't do whole-object validation.validatorAPI is likely to be awkward to use for consumers: for list length validation it works fine, but for other whole-list validation most uses would likely involve reconstructing the list fromindex,removedandadded, which would be painful and bug-prone.minlenandmaxlensupport forListtraits altogether; at that point,TraitListObjectwould become significantly simpler.Key points:
IndexError: for example, givenfoo = List(Int), ifmyobj.foohas length 3, thenmyobj.foo[10] = 4.5will raise aTraitErrorbecause4.5is not a valid int, rather thanIndexErrorbecause10is out of range. This seems to be the most convenient and natural implementation-wise, and matches the current behaviour on master.TraitListObject.__setitem__: for an assignment involving a step that's neither1norNone, Python requires that the object assigned be the same length as the items it's replacing. However, there are tests that expect that the resultingValueErrortakes precedence over anyTraitErrorarising from invalid items, so there's some extra backwards compatibility logic inTraitListObject.__setitem__to ensure those tests pass.This isn't quite ready for merge: there's still some work to do in improving documentation and increasing coverage; while the
trait_list_objectmodule has been almost completely rewritten, I've deliberately tried to leave the tests as unchanged as possible, but I'd like to add more tests to ensure we have good coverage.I'm making the PR now to get feedback from CI and from reviewers.