Settle on Best Practice for Union / Optional Type Annotations
tzaffi opened this issue · 5 comments
Problem
PEP 604 allows the pipe symbol (|
) to annotate unions of types. In particular:
X | Y
Union[X,Y]
and
X | None
Optional[X]
However, type forwarding does not work with this new capability. I.e. "Xtype" | Y
doesn't work (though "Xtype | Y"
does actually work).
Currently our repo is inconsistent when it comes to union types.
Solution
We should settle on a best practice for such annotations, change all usages to adhere to the best practice, and add it to our style guide.
I propose that we disallow |
because of its incompatibility with forwarded types. But I'm open to discussion and don't have a strong opinion, except that we should have some convention.
Dependencies
None
Urgency
Very low
My preference is to use |
everywhere, even if it requires quoting types already available to be included in the quotes e.g:
class MyClass:
def me(self) -> "MyClass | None":
return self
as opposed to:
from typing import Optional
class MyClass:
def me(self) -> Optional["MyClass"]:
return self
Where, obviously, None
by itself would not normally require quoting.
Reasons:
- Brevity in declaration
- Opinion: this makes it easier to read at a glance, I personally find it hard to quickly parse using
Optional
andUnion
especially when they're nested
- Opinion: this makes it easier to read at a glance, I personally find it hard to quickly parse using
- Less chance of requiring to
import typing
in simple cases - Consistency with other languages e.g. TypeScript
- Normally I wouldn't consider this note worthy, but considering there's no PyTeal equivalent for TypeScript currently, I think this bears mentioning.
- Also it's possible for developers from other languages in general to not immediately grok that
Optional[X]
means you can pass an instance of typeX
orNone
, especially ifNone
doesn't follow it as an initial/default value. Optional could imply omitting the value altogether in a function call but that's not necessarily valid unless it has a default value. It's a notion that one will quickly figure out, but again, since this library is something that's likely to be read by developers from multiple languages, it bears mentioning.
I'm convinced by @achidlow's argument. Anyone else care to opine? @michaeldiamant @jasonpaulos @ahangsu
I have no objections
either way's fine with me
I'll follow the majority opinion. I'm less concerned here because the decision feels like it can be changed with minimal friction should a compelling alternative arise later.