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

[BUG] No Optional typing in args that accept None in Console. #1180

Closed
paw-lu opened this issue Apr 22, 2021 · 5 comments · Fixed by #1182
Closed

[BUG] No Optional typing in args that accept None in Console. #1180

paw-lu opened this issue Apr 22, 2021 · 5 comments · Fixed by #1182

Comments

@paw-lu
Copy link
Contributor

paw-lu commented Apr 22, 2021

Some arguments to rich.console.Console—like width—accept None as an argument and are documented as Optional but are typed as only int, raising some type checking errors if width=None is passed.

https://github.com/willmcgugan/rich/blob/a05a5a1c2f95f25db70ac3657e99f0bab652e2cd/rich/console.py#L577

@willmcgugan
Copy link
Collaborator

What type checker are you using? Mypy will assume Optional when you set the default value to None.

@paw-lu
Copy link
Contributor Author

paw-lu commented Apr 22, 2021

"""script.py"""
def foo(x : int = None) -> None:
    ...


foo(x=None)

In this case it fails default Pyre.

❯ pyre
ƛ Using virtual environment site-packages in search path...
ƛ Found 2 type errors!
src/script.py:1:8 Incompatible variable type [9]: x is declared to have type `int` but is used as type `None`.
src/script.py:5:4 Incompatible parameter type [6]: Expected `int` for 1st parameter `x` to call `foo` but got `None`.

It will fail default pytype as well

❯ python -m pytype src/script.py
Computing dependencies
Analyzing 1 sources with 0 local dependencies
ninja: Entering directory `.pytype'
[1/1] check script
FAILED: /Users/pawlu/Documents/scratch/typing-optional/.pytype/pyi/script.pyi
/Users/pawlu/Documents/scratch/typing-optional/.venv/bin/python -m pytype.single --imports_info /Users/pawlu/Documents/scratch/typing-optional/.pytype/imports/script.imports --module-name script -V 3.8 -o /Users/pawlu/Documents/scratch/typing-optional/.pytype/pyi/script.pyi --analyze-annotated --nofail --quick /Users/pawlu/Documents/scratch/typing-optional/src/script.py
File "/Users/pawlu/Documents/scratch/typing-optional/src/script.py", line 5, in <module>: Function foo was called with the wrong arguments [wrong-arg-types]
         Expected: (x: int = ...)
  Actually passed: (x: None)

For more details, see https://google.github.io/pytype/errors.html#wrong-arg-types
ninja: build stopped: subcommand failed.
Leaving directory '.pytype'

You're right that it does pass mypy by default, but it will fail if you use --strict due to it invoking --no-implicit-optional.

❯ python -m mypy --strict src/script.py
mypy.ini: No [mypy] section in config file
src/script.py:1: error: Incompatible default for argument "x" (default has type "None", argument has type "int")
src/script.py:5: error: Argument "x" to "foo" has incompatible type "None"; expected "int"
Found 2 errors in 1 file (checked 1 source file)

For what it's worth, I think mypy plans to (maybe) eventually switch --no-implicit-optional to the default, since PEP 484 was adjusted.

@paw-lu paw-lu mentioned this issue Apr 23, 2021
9 tasks
@paw-lu
Copy link
Contributor Author

paw-lu commented Apr 23, 2021

If you're interested, #1182 attempts to impliment the Optional typing throughout Rich.

@willmcgugan
Copy link
Collaborator

It's somewhat irritating to have a convenient feature added only to have it taken away. However since it looks like you've done the work, I have no issue with changing it.

@paw-lu
Copy link
Contributor Author

paw-lu commented Apr 23, 2021

It's somewhat irritating to have a convenient feature added only to have it taken away.

To be clear the feature is probably staying in mypy (am not a dev, just reading the issues) by explicitly calling --implicit-optional flag, it's just that the default might change, and that the adjustment to PEP 484 no longer recommends assumed optional types. So you could probably pass your own tests using that flag, and future users would turn off implicit optional warnings explicity for rich.

Regardless, thanks for reviewing the PR and thanks for the project!

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

Successfully merging a pull request may close this issue.

2 participants