Skip to content

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jan 12, 2020

This is different from what pre-commit (in "linting") runs in that it
uses our config (alone), and stubs from dependencies.

It would make sense to run this on CI additionally (since there is no
"pre-commit --skip mypy", and a separate config is not worth it).
Currently it triggers a false positive though anyway
(more-itertools/more-itertools#374).

@bluetech
Copy link
Member

This is different from what pre-commit (in "linting") runs in that it uses our config (alone)

Does pre-commit add its own configurations?

and stubs from dependencies.

This will be nice to have, tox linting indeed doesn't include the deps.

IIUC, this currently only includes the non-testing dependencies. But the mypy run also includes testing/, which can import code from testing dependencies, which can have types. I think hypothesis is one. So does it make sense to install these deps too?

args: [--py3-plus]
- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.740
rev: v0.740 # NOTE: keep this in sync with setup.py.
Copy link
Member

Choose a reason for hiding this comment

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

Just for anyone doing the merge from features, note that features is already at v0.761.

setup.py Outdated
]
],
"mypy": [
"mypy==v0.740", # keep this in sync with .pre-commit-config.yaml.
Copy link
Member

Choose a reason for hiding this comment

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

What's the advantage of adding an extra over specifying a deps in [testenv:mypy] tox?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If e.g. pre-commit could also use the extra, the version could be changed just there.
Given requirements.txt, tox.ini etc I often find it more sane to have this in a more central place like setup.py.

Copy link
Member

Choose a reason for hiding this comment

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

OK, makes sense.

I fear though that people will see it and start doing pytest[mypy] due to a misconception. There are probably tools which parse the extras and show them to the user. So WDYT about changing the name to mypy-internal or _mypy or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. What about checkqa-mypy?

Copy link
Member

Choose a reason for hiding this comment

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

That's clear enough I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, amended.

@blueyed
Copy link
Contributor Author

blueyed commented Jan 12, 2020

Does pre-commit add its own configurations?

I thought it would have additional args, but does not have them. So the only diff is with the deps.

Added the testing extra also.

This is different from what pre-commit (in "linting") runs in that it
uses stubs from (test) dependencies.

It would make sense to run this on CI additionally (since there is no
"pre-commit --skip mypy", and a separate config is not worth it).
But currently it triggers a false positive though anyway
(more-itertools/more-itertools#374).
@blueyed blueyed merged commit 4a265ba into pytest-dev:master Jan 14, 2020
@blueyed blueyed deleted the tox-mypy branch January 14, 2020 17:26
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 this pull request may close these issues.

2 participants