Skip to content

set up mypy hook for incremental adoption#199

Merged
neersighted merged 4 commits into
python-poetry:masterfrom
danieleades:mypy
Nov 15, 2021
Merged

set up mypy hook for incremental adoption#199
neersighted merged 4 commits into
python-poetry:masterfrom
danieleades:mypy

Conversation

@danieleades
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@branchv
Copy link
Copy Markdown
Member

branchv commented Nov 14, 2021

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
Copy Markdown
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
Copy Markdown
Contributor Author

@neersighted do you mind triggering the CI for me?

Comment thread poetry/core/_vendor/tomlkit/_utils.py Outdated
branchv
branchv previously approved these changes Nov 14, 2021
Copy link
Copy Markdown
Member

@branchv branchv 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
Copy Markdown
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

Comment thread mypy.ini Outdated
Comment thread .pre-commit-config.yaml Outdated
Comment thread pyproject.toml
Comment thread pyproject.toml Outdated
Comment thread pyproject.toml
Comment thread .pre-commit-config.yaml Outdated
@danieleades
Copy link
Copy Markdown
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.

@neersighted neersighted mentioned this pull request Nov 15, 2021
Comment thread .pre-commit-config.yaml Outdated
neersighted
neersighted previously approved these changes Nov 15, 2021
@neersighted
Copy link
Copy Markdown
Member

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

@danieleades
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@neersighted
Copy link
Copy Markdown
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
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

3 participants