Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
corranwebster opened this issue Sep 30, 2020 · 4 comments · May be fixed by #1786
Open

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

corranwebster opened this issue Sep 30, 2020 · 4 comments · May be fixed by #1786

Comments

@corranwebster
Copy link
Contributor

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.

@corranwebster corranwebster added easy priority: low Low priority for current milestone / sprint type: discussion type: enhancement labels Sep 30, 2020
@mdickinson
Copy link
Member

+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
Copy link
Member

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
Copy link
Contributor

k2bd commented Apr 2, 2024

👋 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
Copy link
Member

👋

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants