Skip to content
This repository has been archived by the owner on Nov 28, 2023. It is now read-only.

feat!: change Optional to IsOptional #142

Closed
wants to merge 1 commit into from
Closed

feat!: change Optional to IsOptional #142

wants to merge 1 commit into from

Conversation

ZanSara
Copy link
Contributor

@ZanSara ZanSara commented Oct 23, 2023

Related to #105 and #111


So far in Canals we've used the type Optional to imply that a specific input value was not to be waited for when a component is ready to run. However, this is in conflict with the actual semantics of Optional, which refers to an input value that can be either of a type or None.

This PR introduces a new type. IsOptional, to separate the two concepts.

Note that IsOptional is an alias of Optional, so mypy may believe that the value can become None. On the other hand, if we don't use IsOptional as an alias of Optional but rather a "no-op" type, some parameters need to be typed this way:

param: IsOptional[Optional[int]] = None

which looks quite odd. Open to suggestions on this regard. A better name for the type might do the trick.


Note. This PR is BREAKING. Components need to be adapted, as with this change all inputs will become mandatory.

OLD SYNTAX:

def run(self, mandatory: int, not_mandatory: Optional[int] = None)

NEW SYNTAX:

def run(self, mandatory: int, not_mandatory: IsOptional[int] = None)

@ZanSara ZanSara requested review from masci and silvanocerza and removed request for masci October 23, 2023 15:22
Copy link
Contributor

@silvanocerza silvanocerza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems even more counter intuitive and confusing that having to specify a default when declaring run().

There's also a difference between Optional and settings a default argument. Optional refers to the type of the argument and should not be mixed up with the optionality of an argument.

Optional tells the user that the value can be None but not that they omit that input.
Defaulting the argument tells the user that they can omit the input, but it doesn't necessarily mean it can be None.

Clashing the two together changes the behaviour of optional inputs too.

This tells me that I can omit the foo input since it has a default, but it still tells me it must be an int.

def run(foo: int = 10)

This instead tells me that I can omit the foo input, but it can also be None. That is not semantically clear. And it's also confusing as it's way too similar to Optional.

def run(foo: IsOptional[int] = 10)

In the end I believe we must not do this.

@ZanSara
Copy link
Contributor Author

ZanSara commented Oct 24, 2023

Yep I eventually reached the same conclusion. I still believe our use of Optional is wrong, but so is this approach. I am going to open another PR using the default value as an indicator instead.

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

Successfully merging this pull request may close these issues.

2 participants