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

Settle on Best Practice for Union / Optional Type Annotations #619

Open
tzaffi opened this issue Dec 19, 2022 · 5 comments
Open

Settle on Best Practice for Union / Optional Type Annotations #619

tzaffi opened this issue Dec 19, 2022 · 5 comments
Labels
new-feature-request Feature request that needs triage Team Scytale

Comments

@tzaffi
Copy link
Contributor

tzaffi commented Dec 19, 2022

Problem

PEP 604 allows the pipe symbol (|) to annotate unions of types. In particular:

X | Y $\equiv$ Union[X,Y]

and

X | None $\equiv$ 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

@tzaffi tzaffi added the new-feature-request Feature request that needs triage label Dec 19, 2022
@achidlow
Copy link
Contributor

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 and Union especially when they're nested
  • 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 type X or None, especially if None 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.

@tzaffi
Copy link
Contributor Author

tzaffi commented Dec 19, 2022

I'm convinced by @achidlow's argument. Anyone else care to opine? @michaeldiamant @jasonpaulos @ahangsu

@jasonpaulos
Copy link
Contributor

I have no objections

@ahangsu
Copy link
Contributor

ahangsu commented Dec 20, 2022

either way's fine with me

@michaeldiamant
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature-request Feature request that needs triage Team Scytale
Projects
None yet
Development

No branches or pull requests

5 participants