enthought / traits

Observable typed attributes for Python classes
Other
436 stars 85 forks source link

Proposal: "Optional(Foo)" as a synonym for "Union(None, Foo)" #1298

Open corranwebster opened 4 years ago

corranwebster commented 4 years ago

It is common to have Union traits of the form Union(None, <something>). As a convenience to developers, and following a similar convention in Python's typing module, we should consider having Optional(<something>) as an alternative way of writing Union(None, <something>).

An alternative would be to make allow_none metadata universally accepted across TraitType instances.

mdickinson commented 4 years ago

+1 for easier ways to spell Union(None, Int()), Union(None, Float()) and Union(None, Str()) - this would make it easier to avoid a class of bugs where the default is used inappropriately.

We'd need to figure out what the recommended practice should be for things that already allow None. Should users ever write Optional(Instance(SomeModel))?

I'm not so keen on the option to extend allow_none to other trait types.

mdickinson commented 4 years ago

Should users ever write Optional(Instance(SomeModel))?

And I'd say "probably yes" here - it's a nice way to indicate to the code reader that yes, it really is intentional that this particular trait will sometimes be None and sometimes have a non-None value.

k2bd commented 7 months ago

👋 Hey, I was surfing for Good First Issue labels to kill an evening and started working on this. A couple questions:

Is that alright if I open a PR here? Is this issue still good / relevant / worth doing? There is a bit of ambiguity in my mind around default values, see the following three cases:

class MyClass(HasTraits):
    a = Optional(Int)
    b = Optional(Int(5))
    c = Optional(Int, default_value=5)

(And this can potentially get weirder with things like Optional(Int(5), default_value=6) depending on interpretation.)

A literal implementation of "Optional(<something>) is a shorthand for Union(None, <something>)" would lead to:

obj = MyClass()

obj.a  # None
obj.b  # None
obj.c  # 5

because only in the third case are you setting the default value of Optional, otherwise you implicitly have the default value of the first type in the union which is None. That seems alright to me in a purely logical interpretation. But in usability the second case visually looks like the intention is to set the default value to 5 and that might be a more intuitive result. Or maybe it's just user error or should be an exception.

What would be the desired behaviour here? And if differing from the above, is there any way to inspect a trait and determine if the default value is set by the user rather than undefined? I.e. differentiate Int(0) vs Int etc, in a robust way? (Maybe I knew this years ago but shamefully it's long gone if so...)

(ETA: Also to note, I think simply flipping the definition to Union(<something>, None) leads to also unintuitive, maybe even more commonly disruptive results; Optional(Int) default would be 0.)

mdickinson commented 6 months ago

👋

Hi Kevin! 👋

Is that alright if I open a PR here?

Absolutely, yes!

Is this issue still good / relevant / worth doing?

I think so, yes.

On defaults, I think I'd go with the straightforward interpretation, so indeed Optional(Int(5)) would have a default of None. I don't think this should be an error, but possibly we could find a way to warn about unused defaults? (Probably out of scope for this particular issue, and ideally such a warning would apply to Union types as well.)

Also to note, I think simply flipping the definition to Union(<something>, None) leads to also unintuitive, maybe even more commonly disruptive results; Optional(Int) default would be 0

Agreed - I think it makes sense for the "default default" for Optional(SomeTraitType) to be None, regardless of what SomeTraitType does.