enthought / enthought/traits

TraitSet, TraitList and TraitDict: validate _all_ incoming values?

Open
#1,020 28 comments 0 reactions 0 assignees View on GitHub
type: discussion
Dominant language
Python
Stars
462
Forks
90
PR merge metrics
No merged PRs in 30d

Description

The current policy in `TraitList`, `TraitSet` and friends is that values that are going to be added to the list/set/whatever are passed through the item validator, while values being removed are not.

This makes some sense: one worry here is that we might do something like this, where `mymodel` is a `HasTraits` instance and `my_set` is a `Set`-based trait on `mymodel`:

```
element = find_matching_element(mymodel.my_set)
do_something_with(element)
mymodel.my_set.remove(element)
```

Here `element` has likely come from direct iteration over the `TraitSet` object, and so should be passed directly to the `remove` call without any transformation. If we passed `element` through the `item_validator` and the validation changed it in some way, so that the `remove` call then failed, that's clearly undesirable.

However, the logic needed to follow this policy in something like `TraitSet.symmetric_difference` [gets really ugly](https://github.com/enthought/traits/blob/456e4d0da519546768bf389984d4538714974fe3/traits/trait_set_object.py#L364-L368), and the result that _some_ of the elements of the r.h.s to `TraitSet.symmetric_difference` will be passed through the validator, while others won't, depending on the exact contents of the set being modified, seems like a recipe for confusion.

I think there may be a better/simpler way, namely:

- assume and document that `item_validator`s should be idempotent: i.e., that for an already validated item `item`, `item_validator(item) == item`. (Probably actually identical rather than just equal in practice, but equality is what matters for collections.) This is already true for all item validators that we care about, I think, including for coercing trait types like `CStr` and potentially value-modifying trait types like `Float` (consider integer inputs to `Float`, for example).
- validate _all_ incoming values to set/list/dict operations. (Credit to @midhun-pm, who originally suggested that we do this, while I originally shot the idea down.)

The interesting side-effect of the second point is that given a `Set(Int)` trait, an operation like `my_set.remove("not an integer")` would raise `TraitError` instead of the expected `KeyError`. The exception in this situation would likely represent a coding bug anyway (rather than being an expected error that people expected to handle), so I _think_ this is a reasonable tradeoff for the extra simplicity and predictability of the `TraitSet` operations.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.