Skip to content

Set up mypy pre-commit hook for incremental adoption#4510

Merged
neersighted merged 3 commits into
python-poetry:masterfrom
danieleades:refactor/mypy
Nov 13, 2021
Merged

Set up mypy pre-commit hook for incremental adoption#4510
neersighted merged 3 commits into
python-poetry:masterfrom
danieleades:refactor/mypy

Conversation

@danieleades
Copy link
Copy Markdown
Contributor

@danieleades danieleades commented Sep 15, 2021

At first glance this pull request might seem a little daft, as i've added a mypy pre-commit hook, and then configured mypy to ignore errors in everything.

There's a method in my madness

this project has many hundreds of type errors and eliminating them would be a significant challenge. A more practical approach is to white-list the current set of files, and then incrementally reduce the scope of that whitelist over time. Or to put it another way, tackle the type errors in one file at a time, and in all new modules.

The plan would be

  1. merge this pull request with a blanket white list of files
  2. accept pull requests which address type errors in specific files. where possible, files should be completely cleared of errors and removed from the whitelist in that pull request
  3. accept future pull requests which make the white list smaller or more specific
  4. one day, possibly, if we're very lucky, the whitelist can be removed entirely

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.

LGTM, with the addition of some explanatory comments and a rebase onto current master.

Comment thread mypy.ini
@neersighted neersighted merged commit d9ba44b into python-poetry:master Nov 13, 2021
@danieleades danieleades deleted the refactor/mypy branch November 13, 2021 15:22
@dimbleby
Copy link
Copy Markdown
Contributor

$ mypy poetry
poetry/layouts/layout.py:123: error: Value of type "Union[Item, Container]" is not indexable
poetry/layouts/layout.py:124: error: Unsupported target for indexed assignment ("Union[Any, Item, Container]")
poetry/layouts/layout.py:125: error: Unsupported target for indexed assignment ("Union[Any, Item, Container]")
poetry/layouts/layout.py:126: error: Unsupported target for indexed assignment ("Union[Any, Item, Container]")
poetry/layouts/layout.py:127: error: Value of type "Union[Any, Item, Container]" is not indexable
poetry/layouts/layout.py:127: error: Item "Item" of "Union[Any, Item, Container]" has no attribute "append"
poetry/layouts/layout.py:127: error: Missing positional argument "item" in call to "append" of "Container"
poetry/layouts/layout.py:130: error: Unsupported target for indexed assignment ("Union[Any, Item, Container]")
poetry/layouts/layout.py:132: error: Item "Item" of "Union[Any, Item, Container]" has no attribute "remove"
poetry/layouts/layout.py:134: error: Unsupported target for indexed assignment ("Union[Any, Item, Container]")
poetry/layouts/layout.py:137: error: Value of type "Union[Any, Item, Container]" is not indexable
poetry/layouts/layout.py:137: error: Item "Item" of "Union[Any, Item, Container]" has no attribute "append"
poetry/layouts/layout.py:137: error: Missing positional argument "item" in call to "append" of "Container"
poetry/layouts/layout.py:137: error: Argument 1 to "append" of "Container" has incompatible type "InlineTable"; expected "Union[Key, str, None]"
poetry/layouts/layout.py:139: error: Item "Item" of "Union[Any, Item, Container]" has no attribute "remove"
poetry/layouts/layout.py:141: error: Value of type "Union[Any, Item, Container]" is not indexable
poetry/layouts/layout.py:141: error: Unsupported target for indexed assignment ("Union[Any, Item, Container]")
poetry/layouts/layout.py:144: error: Value of type "Union[Any, Item, Container]" is not indexable
poetry/layouts/layout.py:144: error: Unsupported target for indexed assignment ("Union[Any, Item, Container]")
poetry/layouts/layout.py:148: error: Value of type "Union[Any, Item, Container]" is not indexable
poetry/layouts/layout.py:148: error: Unsupported target for indexed assignment ("Union[Any, Item, Container]")
poetry/layouts/layout.py:152: error: Item "Item" of "Union[Any, Item, Container]" has no attribute "__delitem__"
poetry/layouts/layout.py:168: error: Argument 2 to "add" of "Container" has incompatible type "Table"; expected "Optional[Item]"
poetry/layouts/layout.py:170: error: Incompatible types in assignment (expression has type "str", variable has type "TOMLDocument")
poetry/layouts/layout.py:173: error: Incompatible types in assignment (expression has type "str", variable has type "TOMLDocument")
poetry/layouts/layout.py:173: error: Unsupported operand types for + ("str" and "TOMLDocument")
poetry/layouts/layout.py:175: error: Incompatible return value type (got "TOMLDocument", expected "str")
Found 27 errors in 1 file (checked 153 source files)

Not sure why poetry.layouts is not in the list of files that mypy is allowed to ignore?

That this was committed without some hook noticing and complaining suggests that what you are trying to do - which I am all in favour of! - is not working. Suggestions:

  • add mypy to poetry's dev-dependencies so that local developers are sure to have it available to them
  • github workflow should be executing mypy

@neersighted @danieleades

@danieleades
Copy link
Copy Markdown
Contributor Author

Hi @dimbleby, mypy is being run by a GitHub workflow, it's a pre-conmit hook.

The pre-commit hook passes some additional command line arguments too, so the command won't be quite equivalent to what you're running manually. For local development you probably want

pre-commit run --all-files

@dimbleby
Copy link
Copy Markdown
Contributor

Thanks, I dug in a bit further. The difference is that the virtual environment in which precommit runs mypy does not include tomlkit - so it simply sees a missing import, and is configured to ignore that.

Whereas running mypy in the proper environment, it does know about tomlkit and does recognise all those type errors.

Perhaps I am still misunderstanding precommit, but if it is really running mypy in an environment that does not include project dependencies, that seems as though it will hurt mypy's ability to spot problems?

@dimbleby
Copy link
Copy Markdown
Contributor

dimbleby commented Nov 13, 2021

Here is a blog post that seems to confirm that this time I am onto something, and proposing some workarounds / solutions.

@danieleades
Copy link
Copy Markdown
Contributor Author

Thanks @dimbleby, I'll have a dig.

There's no question the current configuration is missing a lot of errors- that's not entirely an accident. The plan is to tighten up the configuration incrementally over time. Would be great to see a pull request from you plugging some of these gaps!

@dimbleby
Copy link
Copy Markdown
Contributor

Yeah, I'll pick off a couple of the trivial ones to show good faith...!

I have mostly found contributing to poetry a somewhat disheartening experience, but perhaps better luck this time.

@dimbleby
Copy link
Copy Markdown
Contributor

#4751

@danieleades
Copy link
Copy Markdown
Contributor Author

I have mostly found contributing to poetry a somewhat disheartening experience

See my "governence" issue...

#4595

1nF0rmed pushed a commit to 1nF0rmed/poetry that referenced this pull request Nov 15, 2021
neersighted pushed a commit to python-poetry/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
danieleades added a commit to danieleades/poetry-core that referenced this pull request Nov 15, 2021
@github-actions
Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants