Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

Arguments with type hint Optional are incorrect #377

Closed
jakebailey opened this issue Nov 8, 2018 · 5 comments
Closed

Arguments with type hint Optional are incorrect #377

jakebailey opened this issue Nov 8, 2018 · 5 comments
Assignees
Milestone

Comments

@jakebailey
Copy link
Member

jakebailey commented Nov 8, 2018

image

Should really one of:

opt(x: Optional[int]) -> int, None
opt(x: int, None) -> int, None

image

If I'm following the same format as my previous "should", then this should really be one of:

opt(x: Optional[int] = None) -> int, None
opt(x: int, None = None) -> int, None

But the latter (for both) is nonsensical and really ambiguous when it comes to parameters which are separated by commas, but likely how we'd render things with our current type system. Type alternatives really shouldn't be rendered like this. Related to #346.

@jakebailey
Copy link
Member Author

The return type being int, None is probably not PEP 484 compliant either, but I'd need to read the spec to see what's expected there. I'm guessing Optional[int] is also the right return type, too...

@dirn
Copy link

dirn commented Nov 11, 2018

In this particular example, the return type should be wrapped in Tuple.

def opt_none(x: Optional[int] = None) -> Tuple[Optional[int], None]

@jakebailey
Copy link
Member Author

jakebailey commented Nov 11, 2018

Our current way of displaying types separates alternates with commas, which is why I wrote int, None in my original post. The return type should be Optional[int] as the parameter has, not a tuple. If I wrote return x, None, then it would be Tuple[Optional[int], None].

Forgive my mixing of different syntaxes. There's already an issue open for trying to transition to a more PEP 484 style system rather than our existing one (#346), I was just avoiding mixing two different issues in favor of limiting this one to the Optional bug.

@dirn
Copy link

dirn commented Nov 11, 2018

Yeah, sorry for not being clear, I was specifically addressing this part

The return type being int, None is probably not PEP 484 compliant either

and hadn't seen the other issue. And your example doesn't return a tuple, like you said, so I'm not sure what I was even thinking. Ignore me. :)

@jakebailey
Copy link
Member Author

Closing this, since the new language server's typing system is much more aligned with Python's typing and weird things like this don't show up in the analysis anymore. The only things left are small tooltip tweaks, like #603.

@jakebailey jakebailey added this to the Feb 2019.1 milestone Feb 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants