Skip to content

CI: Unpin MyPy #36012

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

Merged
merged 6 commits into from
Sep 1, 2020
Merged

CI: Unpin MyPy #36012

merged 6 commits into from
Sep 1, 2020

Conversation

simonjayhawkins
Copy link
Member

No description provided.

@simonjayhawkins simonjayhawkins added CI Continuous Integration Typing type annotations, mypy/pyright type checking labels Aug 31, 2020
# NOTE: tokenize.Name is not a public constant
# error: Module has no attribute "Name" [attr-defined]
if not re.match("^" + tokenize.Name + "$", k): # type: ignore[attr-defined]
if not re.match("^" + tokenize.Name + "$", k):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

latest typeshed (bundled with mypy) exposes undocumented attributes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean theres a lower bound for what mypy we want?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that in general it is recommended to pin mypy see https://mypy.readthedocs.io/en/stable/existing_code.html?highlight=pin#continuous-integration

does this mean theres a lower bound for what mypy we want?

with warn_unused_ignores, yes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I could change to mypy=0.782 (best practice) or mypy>=0.782 (see #27568 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented above, prefer either of these to no version

# "Union[pandas.core.common.<subclass of "Iterable" and "Sized">,
# pandas.core.common.<subclass of "Iterable" and "Sized">1, T]", expected
# "Union[Collection[T], T]") [return-value]
obj = cast(Collection, obj)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our signature here is not entirely correct. If we pass an Iterable with a __len__ method which is not necessarily a collection, the object is returned unchanged.

I think best to leave signature unchanged for now and could use Protocol in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since i'm lying to the type checker a #type: ignore is probably more preferable to a cast.

@@ -364,7 +364,7 @@ class BaseExprVisitor(ast.NodeVisitor):

unary_ops = _unary_ops_syms
unary_op_nodes = "UAdd", "USub", "Invert", "Not"
unary_op_nodes_map = dict(zip(unary_ops, unary_op_nodes))
unary_op_nodes_map = {k: v for k, v in zip(unary_ops, unary_op_nodes)}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy giving

error: Need type annotation for 'unary_op_nodes_map' (hint: "unary_op_nodes_map: Dict[, ] = ...") [var-annotated]

mypy is ok with a dictcomp, or could add Dict[str, str] as variable type annotation instead

__setitem__ = __setslice__ = _disabled # type: ignore[assignment]
__delitem__ = __delslice__ = _disabled # type: ignore[assignment]
pop = append = extend = _disabled # type: ignore[assignment]
remove = sort = insert = _disabled # type: ignore[assignment]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably could make FrozenList Generic to allow type annotations such as FrozenList[Label]

This is an exercise for another day.

[mypy-pandas.conftest]
ignore_errors=True

[mypy-pandas.tests.tools.test_to_datetime]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errors were fixed with latest typeshed

ignore_errors=True

[mypy-pandas.tests.tools.test_to_datetime]
[mypy-pandas.conftest,pandas.tests.window.conftest]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar error in pandas.tests.window.conftest as in pandas.conftest,pandas, so bundled together.

this is a mypy inference issue we have encountered before

error: List item 0 has incompatible type "ParameterSet"; expected "Sequence[Collection[object]]" [list-item]

setup.cfg Outdated
@@ -226,6 +277,9 @@ check_untyped_defs=False
[mypy-pandas.io.excel._util]
check_untyped_defs=False

[mypy-pandas.io.excel._xlsxwriter]
check_untyped_defs=False

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errors in this module fixed in #35995

@jbrockmendel
Copy link
Member

LGTM

@simonjayhawkins simonjayhawkins added this to the 1.2 milestone Aug 31, 2020
@jbrockmendel
Copy link
Member

LGTM

environment.yml Outdated
@@ -21,7 +21,7 @@ dependencies:
- flake8-comprehensions>=3.1.0 # used by flake8, linting of unnecessary comprehensions
- flake8-rst>=0.6.0,<=0.7.0 # linting of code blocks in rst files
- isort>=5.2.1 # check that imports are in the right order
- mypy=0.730
- mypy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer we pin exactly for this, or at the very least a lower bound.

# NOTE: tokenize.Name is not a public constant
# error: Module has no attribute "Name" [attr-defined]
if not re.match("^" + tokenize.Name + "$", k): # type: ignore[attr-defined]
if not re.match("^" + tokenize.Name + "$", k):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented above, prefer either of these to no version

@WillAyd
Copy link
Member

WillAyd commented Aug 31, 2020

lgtm ex @jreback comments; definitely need to get this updated given all of the changes between 0.73 and now

@jreback jreback merged commit 67429d4 into pandas-dev:master Sep 1, 2020
@jreback
Copy link
Contributor

jreback commented Sep 1, 2020

thanks @simonjayhawkins

note may want to add this to the whatsnew for development changes (we usually list pytest version changes)

@simonjayhawkins simonjayhawkins deleted the unpin-mypy branch September 2, 2020 09:21
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants