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

Replace Optional[T] with T for default args, that expect T and not None #82

Closed
sayandipdutta opened this issue Feb 2, 2022 · 2 comments · Fixed by #89
Closed

Replace Optional[T] with T for default args, that expect T and not None #82

sayandipdutta opened this issue Feb 2, 2022 · 2 comments · Fixed by #89
Assignees

Comments

@sayandipdutta
Copy link
Contributor

sayandipdutta commented Feb 2, 2022

In quite a few places, function parameters with defaults are annotated with typing.Optional[T] (where T could be str, int, ...), I propose changing these to T where it is deemed to be the expected type of the variable for the rest of the block, and explicit None is not valid.

typing.Optional docs says the following about default value type annotations.

Note that this is not the same concept as an optional argument, which is one that has a default. An optional argument with a default does not require the Optional qualifier on its type annotation just because it is optional.


This is not merely due to aesthetics, in my experience, Optional qualifiers can create some annoying type checking issues with mypy. For example:
test.py

from typing import Optional
def foo(arg: str) -> None:
    pass

def bar(arg: Optional[str] = "") -> None:
    foo(arg)
    return

Running mypy:

> mypy .\test.py
.\test.py:6: error: Argument 1 to "foo" has incompatible type "Optional[str]"; expected "str"
Found 1 error in 1 file (checked 1 source file)

Then you will need to do one of the following:

from typing import cast
def bar(arg: Optional[str] = "") -> None:
    foo(cast(str, arg))
    return

or

def bar(arg: Optional[str] = "") -> None:
    assert arg is not None
    foo(arg)
    return

or

def bar(arg: Optional[str] = "") -> None:
    if isinstance(arg, str):
        foo(arg)
    return

Although a single cast , assert or isinstance may not be a big deal, if you have to use it repeatedly, especially in loops, it looks unclean and reduces performance.


Originally posted by @sayandipdutta in #75 (comment)

Related: #75 (comment)

@FyzHsn
Copy link
Member

FyzHsn commented Feb 3, 2022

@sayandipdutta If you'd like to work on this or have started already, assign yourself to this issue.

@sayandipdutta
Copy link
Contributor Author

sayandipdutta commented Feb 3, 2022

@FyzHsn I will start working on it tomorrow. But I don't have permission to assign this to myself.

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 a pull request may close this issue.

2 participants