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

Add "| None" type annotation for args that can be None #388

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

siddancha
Copy link
Contributor

@siddancha siddancha commented Nov 20, 2024

Currently, many arguments are annotated as type Type (e.g. Meshcat) while their default value is set to None. This "implicit optional" convention, which was acceptable in the past, is now discouraged by PEP-484 [1]. It causes my editor to show "unreachable code" warnings since it assumes these arguments cannot be None.


This PR fixes the type annotation from Type to Type | None.

There is an older way to denote typing.Union[Type, None], which is typing.Optional[Type]. Type | None was introduced in PEP-604. Both are valid. There is no precedent in the manipulation repo, while both have been used in Drake [2, 3].

However, Type | None seems to be encouraged by Python developers [4, 5] over typing.Optional.


This change is Reviewable

Copy link
Collaborator

@nepfaff nepfaff left a comment

Choose a reason for hiding this comment

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

In the past, I used Optional for optional arguments that are None by default and Union for everything else. Would you say people are moving towards using Type | None for everything?

Thanks for doing this! :lgtm:

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @siddancha)

@nepfaff nepfaff merged commit 1875761 into RussTedrake:master Nov 20, 2024
6 checks passed
Copy link
Contributor Author

@siddancha siddancha left a comment

Choose a reason for hiding this comment

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

@nepfaff I don't know if people are moving to X | None. But ideally they should be.

  • Using the shorthand X | Y is recommended over Union[X, Y] in the official documentation [1]. And Optional[X] is identical to Union[X, None].

image.png

  • Here's an extensive discussion on Python's forums [2]. The takeaway is that the term Optional is badly named. For example, in def f(x: int = 3, y: Optional[int] = None), x is as much an optional argument as y, while having nothing to do with None. Semantically, Optional[X] means exactly the same as Union[X, None] to the type-checker. To quote the link:

If we were writing PEP 484 from scratch in 2023, I don’t think we would include the Optional symbol in the typing module. We would simply tell people to use Union[X, None] to express “it could be X, or it could be None”.

Having said that, Optional was not deprecated by PEP-604, and there are no plans to do it anytime soon. So I think it's fine to consistently use either Optional[X] or X | None on a per-project basis.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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

Successfully merging this pull request may close these issues.

2 participants