@jdfreder

@SylvainCorlay I think this generally is a good idea. Since passing a callback in as metadata is atypical for the traitlets module, I'm going to ping @ellisonbg to get his opinion. My own opinion is that I like the design, but that may be because it's Javascript-like. :)

I think that there is a great need to allow the HasTraits instance to hook up its own logic in the validation stage.

I think that there is a great need to allow the HasTraits instance to hook up its own logic in the validation stage.

Definitely!

+1 to the idea. I think it would make sense to make the HasTraits validation come after the traitlet validation. For example, the traitlet validation may cast a value to a specific type, etc. That can be useful to do before the HasTraits validation function runs.

For example, suppose I have a CDate function that understands lots of string date formats and casts them to standard python date formats. It's next to useless to have a HasTraits validation run before the traitlet validation since the HasTraits validation doesn't understand the string formats. However, once the traitlet validation has run, there are some guarantees about what is in the value, and the HasTraits validation function can rely on that.

@jasongrout Good idea. This would simply require moving the two lines of code after the validation. Let's wait for other people's feedback.

Besides, as suggested by @jasongrout in person, we could remove the validation with
is_valid_for and value_for which are not used anywhere.
Should we make a separate PR?

Isn't this already possible using subclassing? i.e. define a new EvenInt traitlet class, subclassing from Int, and override its validate method. I don't think we should add too many hooks to achieve the same thing in different ways, because it makes the logic harder to follow.

This would require to make a new subclass of Int for each subclass of HasTrait that has its own validation logic for Int attributes, and a different one for each one of its Int attribute, unless that subclass implements something along these lines. In other words, it is probably possible to do it with inheritance, but unpractical.

@takluyver - this proposal allows a HasTraits class to cleanly validate traitlets against each other. To do this at the traitlet level, how would you change the validation of the value attribute when the parity attribute changed?

We face the same issue in the slider: how do we validate the value against the max and min? We don't want to set the value and then recant the setting if it was actually invalid to start with.

As per discussion,

• removed the unused is_valid_for and value_for validation stages and their test cases.
• moved the custom traitlet hook to be the last validation stage as per Jason's comment.

ping @ellisonbg

So I don't forget: I think this would be neater if the API involved defining a _foo_validate(self, value) method on the class the traitlet is attached to (analogous to _foo_default and _foo_changed), rather than passing a function to the traitlet itself as metadata.

@takluyver 👍

Changed the API according to Thomas' proposition. Here is the working example.

from IPython.utils.traitlets import *

class Parity(HasTraits):

    value = Int(0)
    parity = Enum(['odd', 'even'], default_value='even', allow_none=False)

    def _value_validate(self, value, trait):
        if self.parity == 'even' and value % 2:
            raise TraitError('Expected an even number')
        if self.parity == 'odd' and (value % 2 == 0):
            raise TraitError('Expected an odd number')
        return value 

u = Parity()

u.parity = 'odd'
u.value = 1 # OK
u.value = 2 # Should raise a TraitError

@takluyver do you have any more thoughts on this?

It looks fine at a glance. We'll look at it in more detail once 3.0 is out.

Rebased - just to reflect the latest changes in traitlets.

Added the example as a test.

This has run afoul of the complex Qt mixins, causing:

  File ".../IPython/utils/traitlets.py", line 460, in _validate
    if hasattr(obj, '_%s_validate' % self.name):
RuntimeError: super-class __init__() of type RichIPythonWidget was never called

I'm not sure yet how problematic the behavior is. It might be okay to just swallow the error, it might not.

Do you have a traceback of this? Btw, with #7770, we will be less likely to try to validate default values at init time.

I fixed it with #8099. Since it was only the case of missing attributes raising RuntimeError instead of AttributeError, a slightly broader catch seems to be fine. We can get #7770 in soon, and hopefully that will make it simpler.