-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
TYP: Make mypy 0.800 compatible #39407
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
Conversation
phofl
commented
Jan 25, 2021
- closes TYP upgrade and pin mypy to 0.800 #39399
- Ensure all linting tests pass, see here for how to run them
LGTM cc @simonjayhawkins |
environment.yml
Outdated
@@ -23,7 +23,7 @@ dependencies: | |||
- flake8 | |||
- flake8-comprehensions>=3.1.0 # used by flake8, linting of unnecessary comprehensions | |||
- isort>=5.2.1 # check that imports are in the right order | |||
- mypy=0.790 | |||
- mypy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think ok to pin >=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have been pinning to exact version up to now.
we pin to avoid ci surprises. also since we use warn_unused_ignores = True
, mypy will only be green with the specified version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, pinned again
@@ -28,7 +28,7 @@ class BoxPlot(LinePlot): | |||
|
|||
_valid_return_types = (None, "axes", "dict", "both") | |||
# namedtuple to hold results | |||
BP = namedtuple("Boxplot", ["ax", "lines"]) | |||
BP = namedtuple("BP", ["ax", "lines"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OT we could use data classes here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @phofl generally lgtm
environment.yml
Outdated
@@ -23,7 +23,7 @@ dependencies: | |||
- flake8 | |||
- flake8-comprehensions>=3.1.0 # used by flake8, linting of unnecessary comprehensions | |||
- isort>=5.2.1 # check that imports are in the right order | |||
- mypy=0.790 | |||
- mypy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have been pinning to exact version up to now.
we pin to avoid ci surprises. also since we use warn_unused_ignores = True
, mypy will only be green with the specified version.
pandas/io/common.py
Outdated
@@ -182,7 +182,7 @@ def stringify_path( | |||
return cast(FileOrBuffer[AnyStr], filepath_or_buffer) | |||
|
|||
# Only @runtime_checkable protocols can be used with instance and class checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pandas/io/stata.py
Outdated
@@ -2491,9 +2491,9 @@ def write_file(self) -> None: | |||
self.handles.close() | |||
# Only @runtime_checkable protocols can be used with instance and class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -79,7 +79,7 @@ def wrapper(*args, **kwargs) -> Callable[..., Any]: | |||
{dedent(doc)}""" | |||
) | |||
|
|||
return wrapper | |||
return wrapper # type: ignore[return-value] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the error message as a comment to help future readers and contributors looking to help with #37715
return wrapper # type: ignore[return-value] | |
# error: Incompatible return value type (got "Callable[[VarArg(Any), | |
# KwArg(Any)], Callable[...,Any]]", expected "Callable[[F], F]") | |
return wrapper # type: ignore[return-value] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -330,8 +331,9 @@ def test_read_csv_file_handle(all_parsers, io_class, encoding): | |||
parser = all_parsers | |||
expected = DataFrame({"a": [1], "b": [2]}) | |||
|
|||
content = "a,b\n1,2" | |||
content: Union[str, bytes] = "a,b\n1,2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
content: Union[str, bytes] = "a,b\n1,2" | |
content: str | bytes = "a,b\n1,2" |
😁
we have
[mypy-pandas.tests.*]
check_untyped_defs=False
so not sure why mypy is checking this function anyway.
OTOH could refactor...
content = "a,b\n1,2"
handle = io_class(content.encode("utf-8") if io_class == BytesIO else content)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm the refactor is better. Thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @phofl lgtm pending green
@simonjayhawkins green. Could you point me to an example of something like |
from http://mypy-lang.blogspot.com/ Two new Python features improve this situation and are now supported by mypy: PEP 585 lets you use list[int] instead of List[int] (no need to import List and other generic collections from typing). |
thanks very much, should have looked there and not in our codebase :) |
@phofl also need to update the version in the doc/source/whatsnew/v1.3.0.rst |
Thx, did not think about this. #39419 |