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

set up mypy hook for incremental adoption #199

Merged
merged 4 commits into from Nov 15, 2021

Conversation

danieleades
Copy link
Contributor

as per the comment here - python-poetry/poetry#4510

for some reason i had to use different config to successfully exclude the _vendor directory

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Same as on poetry -- lets add some comments and rebase to fix the merge conflicts.

neersighted
neersighted previously approved these changes Nov 13, 2021
@danieleades
Copy link
Contributor Author

@neersighted this should be good to go. Not sure what's caused that python 3.10 workflow failure

@branchvincent
Copy link
Member

The mypy error is an open issue (see python/mypy#1393) and only occurs on 3.10 since collections.Mapping was removed (8th bullet). We could either:

  1. Ignore it with # type: ignore
  2. Replace it with a non-guarded from collections.abc import Mapping, given it's unneeded compatibility for python 2

@neersighted
Copy link
Member

Go ahead with collections.abc -- we're slowly trying to rip out Python 2 compatibility from poetry and poetry core -- there's no reason not to use the correct import path these days.

@danieleades
Copy link
Contributor Author

@neersighted do you mind triggering the CI for me?

branchvincent
branchvincent previously approved these changes Nov 14, 2021
Copy link
Member

@branchvincent branchvincent left a comment

Choose a reason for hiding this comment

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

fwiw, this looks good to me

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Please note that when I say 'like in Poetry,' I really mean 'like in the linked PR' that I am working to get merged.

python-poetry/poetry#4750

mypy.ini Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@danieleades
Copy link
Contributor Author

i've aligned the pre-commit config with the pyproject.toml file. my only concern is that the blanket allow_missing_imports flag might be slightly too unrestrictive/untargeted. Then again, it appears to be baked into the pre-commit hook.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
neersighted
neersighted previously approved these changes Nov 15, 2021
@neersighted
Copy link
Member

Looks like we get a failure on pypy, which is interesting. Is this rebased onto the latest master?

@danieleades
Copy link
Contributor Author

Looks like we get a failure on pypy, which is interesting. Is this rebased onto the latest master?

it is now

@danieleades
Copy link
Contributor Author

Looks like we get a failure on pypy, which is interesting. Is this rebased onto the latest master?

python/typed_ast#111

@danieleades
Copy link
Contributor Author

rebased on master again. let's see if that helps

@neersighted
Copy link
Member

neersighted commented Nov 15, 2021

Looks like we get a failure on pypy, which is interesting. Is this rebased onto the latest master?

python/typed_ast#111

I think the better thing to do here is to not depend on mypy in pyproject.toml at all -- we run it only under pre-commit anyway. If the user chooses to install mypy separately, that can be their (partially unsupported) business I think.

Alternatively, we could add a marker to only install on CPython (see the black dep above) -- but I'm thinking we should just remove the black and isort deps in the future and rely on pre-commit as the main poetry repo does.

@danieleades
Copy link
Contributor Author

Looks like we get a failure on pypy, which is interesting. Is this rebased onto the latest master?

python/typed_ast#111

I think the better thing to do here is to not depend on mypy in pyproject.toml at all -- we run it only under pre-commit anyway. If the user chooses to install mypy separately, that can be their (partially unsupported) business I think.

Alternatively, we could add a market to only install on CPython (see the black dep above) -- but I'm thinking we should just remove the black and isort deps in the future and rely on pre-commit as the main poetry repo does.

i've removed the mypy dep for now.

@neersighted neersighted merged commit abc5640 into python-poetry:master Nov 15, 2021
@danieleades danieleades deleted the mypy branch November 15, 2021 15:16
danieleades added a commit to danieleades/poetry-core that referenced this pull request Nov 15, 2021
danieleades added a commit to danieleades/poetry-core that referenced this pull request Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants