Instance trait type default value confusion

[rmorshea]

We raise here:

class A(HasTraits):
    inst = Instance(Foo)

A().inst

But not here:

class A(HasTraits):
    inst = Instance(Foo, allow_none=True)

A().inst

This seems magical, and is not at all implied by allow_none=True.

We should probably try to enforce this convention:

class A(HasTraits):
    inst = Instance(Foo, default_value=None, allow_none=True)

A().inst

Or at the very least:

set allow_none=True if default_value=None

class A(HasTraits):
    inst = Instance(Foo, default_value=None)

A().inst

This isn't backwards compatible, but IMO it resolves a bug.

[rmorshea]

@minrk, this needs to be resolved before #317 is merged. However doing so will result in backward incompatible changes to subclasses of Instance, but similarly to #342, I feel that this use of "invalid" defaults via make_dynamic_default is an exploit. I will attempt to put together a comprehensive set of pr's across the notebook.

[rmorshea]

@minrk, thoughts on this?

[minrk]

This seems magical, and is not at all implied by allow_none=True.

I don't agree with this. It's not magical to have default values for arguments.

I don't see an issue with the current behavior on this. A trait isn't validated until it gets a value (this is deliberate and important). Accessing that value the first time will cause its initialization, which if a value has not been assigned and the default value is not allowed, will raise.

I strongly disagree with this:

We should probably try to enforce this convention:

 class A(HasTraits):
     inst = Instance(Foo, default_value=None, allow_none=True)

That's an extremely verbose signature for a very common, supposedly simple case, especially one that was the default behavior for years. When simple, common things are complicated, the design is probably not right. This is an improvement:

class A(HasTraits):
    inst = Instance(Foo, default_value=None)

but ultimately, not an improvement over the current behavior of:

class A(HasTraits):
    inst = Instance(Foo, allow_none=True)

We also have to take into account that changing this would break lots of existing code throughout IPython, which we simply cannot do here except over multiple major releases and long deprecation cycles.

What I would really prefer is to switch Instance back to its original default allow_none=True, matching the longstanding behavior of traits. Switching to allow_none=False by default caused a lot of problems, and doesn't appear to have improved the experience of using traitlets.

[rmorshea]

"Magical" was probably not the right word. "Imprecise" would have been better.

I agree that

 class A(HasTraits):
     inst = Instance(Foo, default_value=None, allow_none=True)

is much too verbose, but I'm not convinced that

class A(HasTraits):
    inst = Instance(Foo, allow_none=True)

is an improvement over

class A(HasTraits):
    inst = Instance(Foo, default_value=None)

since the latter shows, rather than implies, that the default value will be None.

[ankostis]

I would also vote for default_value=None; it is more obvious what is happening. I keep hitting this dilemma every time i have to use a none-instance, and by heart, i choose this one, which is wrong :-(.