Force type hints #233

finswimmer · 2021-11-15T05:40:15Z

This PR add missing type hints to both package code and test code. Furthermore flake-annotations is added as a flake8 plugin to enforce type hints in the future.

neersighted · 2021-11-15T06:32:51Z

.flake8

@@ -1,7 +1,11 @@
 [flake8]


This whole file is very cargo-culted. Can we clean it up while we're in here?

.flake8

.pre-commit-config.yaml

neersighted · 2021-11-15T06:35:31Z

poetry/core/version/pep440/version.py

@@ -46,7 +47,21 @@ def __post_init__(self):

        object.__setattr__(self, "_compare_key", self._make_compare_key())

-    def _make_compare_key(self):
+    def _make_compare_key(


Wow, this is a complicated type. Is there any way to clean this up with NewType, a refactor with an object, or similar?

I was happy, that PyCharm was doing this for me 😆 IMO refactoring should be done in another PR, because the purpose of this PR is to enforce type hints in the future.

That is a brutal type signature :)

I've tried to understand the code and IMO the signature can be written more easily. See code change.

danieleades

mypy itself can be configured to return errors on missing annotations, i believe. is the 'flake8' plugin somehow preferable?

if TYPE_CHECKING pattern doesn't appear to be required throughout
inconsistent use of dict/Dict

danieleades · 2021-11-15T06:43:55Z

tests/conftest.py

@@ -10,7 +11,12 @@
 from tests.testutils import tempfile


-def pytest_addoption(parser):
+if TYPE_CHECKING:


you shouldn't need the if TYPE_CHECKING pattern here- that's really only for preventing circular imports. You can just import these directly, and use them below without quoting the types

That's a convention I'm used to. If you need a module just for type hints, import it only in the if TYPE_CHECKING block.

It's easier to teach to other people ("Why are you sometime put import in that block and sometimes not?") and the overall chance that you have a circular import if you need this import just for type hint is quite big.

TL;DR: I prefer to leave it as it is.

fair enough. there's some interesting background on it here - larryhastings/co_annotations#1

in general, it seems that if TYPE_CHECKING is used iff

you would otherwise have a circular import

the import is 'expensive'

the Benevolent Dictator for Life seems to regard it is a painfully necessary hack.

that said, it's also clear from the thread that it's far from settled. And using it everywhere instead of the small number of cases where you need it (do you definitely need it anywhere in this codebase?) is at least consistent.

danieleades · 2021-11-15T06:44:23Z

tests/conftest.py

@@ -20,15 +26,15 @@ def pytest_addoption(parser):
    )


-def pytest_configure(config):
+def pytest_configure(config: "Config") -> None:


you can use Config unquoted if you remove the if TYPE_CHECKING pattern above

danieleades · 2021-11-15T06:47:08Z

tests/json/test_poetry_schema.py

@@ -4,7 +4,7 @@


 @pytest.fixture
-def base_object():
+def base_object() -> dict:


this syntax is python 3.9+. less than that, the correct return type would be Dict, rather than dict.

I guess we can get away with it, since mypy is being run with a recent interpreter, and the type is ignored during other testing, but whichever way we go, it should be consistent throughout the codebase

while we're at it, these dict annotations are equivalent to Dict[Any, Any]- we should be able to do better than that

As you find out dict is valid as well, but don't let us define how keys and values look like in python<3.9. I agree it's in inconsistent use if we use Dict in other places, so I changed it.

while we're at it, these dict annotations are equivalent to Dict[Any, Any]- we should be able to do better than that

The only way to do it better here is to use Dict[str, Any] (which doesn't add much benefit) or use TypedDict. But this is only available for python >=3.8 and would require to add typing-extensions as dependency for python <3.8. Not sure if we want to introduce another dependency.

danieleades · 2021-11-15T06:47:49Z

tests/masonry/builders/test_builder.py

@@ -10,7 +13,11 @@
 from poetry.core.utils._compat import PY37


-def test_builder_find_excluded_files(mocker):
+if TYPE_CHECKING:


is there a circular import if you do not guard the import of MockerFixture?

tests/testutils.py

danieleades · 2021-11-15T11:53:58Z

poetry/core/version/pep440/parser.py

+    def parse(
+        cls, value: str, version_class: Optional[Type["PEP440Version"]] = None
+    ) -> "PEP440Version":


if you remove the if TYPE_CHECKING guard, this can be written as-

Suggested change

def parse(

cls, value: str, version_class: Optional[Type["PEP440Version"]] = None

) -> "PEP440Version":

def parse(

cls, value: str, version_class: Optional[PEP440Version] = None

) -> PEP440Version:

danieleades · 2021-11-15T13:14:45Z

might be a good idea to merge #199 before this is merged, in order to catch any type errors that could be interested by this PR

neersighted · 2021-11-15T13:38:42Z

re: if TYPE_CHECKING, check out https://github.com/asottile/flake8-typing-imports

Looks like a useful set of lints we could apply here.

finswimmer force-pushed the improvement/force-type-hints branch from eb9b5c2 to b7e82ba Compare November 15, 2021 06:17

finswimmer marked this pull request as ready for review November 15, 2021 06:30

finswimmer requested a review from a team November 15, 2021 06:31

neersighted requested changes Nov 15, 2021

View reviewed changes

danieleades suggested changes Nov 15, 2021

View reviewed changes

danieleades reviewed Nov 15, 2021

View reviewed changes

tests/testutils.py Show resolved Hide resolved

finswimmer force-pushed the improvement/force-type-hints branch from 17e23a7 to 36ac1ce Compare November 15, 2021 10:35

danieleades reviewed Nov 15, 2021

View reviewed changes

finswimmer force-pushed the improvement/force-type-hints branch 2 times, most recently from a8847b5 to de16fac Compare November 15, 2021 13:06

finswimmer requested a review from neersighted November 15, 2021 13:11

finswimmer force-pushed the improvement/force-type-hints branch from de16fac to eb6c089 Compare November 15, 2021 16:40

finswimmer added 5 commits November 16, 2021 11:30

change type annotations to type hints using com2ann

2461a7b

add missing type hints to core

15c2bc3

add type hints to tests

cc70d69

add missing type hints to stanza

75d6ef6

add flake8-annotations to flake8 pre-commit hook

38bbcff

finswimmer force-pushed the improvement/force-type-hints branch from eb6c089 to 38bbcff Compare November 16, 2021 10:32

neersighted approved these changes Nov 16, 2021

View reviewed changes

finswimmer merged commit 4d119fd into python-poetry:master Nov 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force type hints #233

Force type hints #233

finswimmer commented Nov 15, 2021

neersighted Nov 15, 2021 •

edited

Loading

neersighted Nov 15, 2021

finswimmer Nov 15, 2021

neersighted Nov 15, 2021

finswimmer Nov 15, 2021

danieleades left a comment

danieleades Nov 15, 2021

finswimmer Nov 15, 2021

danieleades Nov 15, 2021

danieleades Nov 15, 2021

danieleades Nov 15, 2021

danieleades Nov 15, 2021

finswimmer Nov 15, 2021

danieleades Nov 15, 2021

danieleades Nov 15, 2021

danieleades commented Nov 15, 2021

neersighted commented Nov 15, 2021 •

edited

Loading

Force type hints #233

Force type hints #233

Conversation

finswimmer commented Nov 15, 2021

neersighted Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieleades left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieleades commented Nov 15, 2021

neersighted commented Nov 15, 2021 • edited Loading

neersighted Nov 15, 2021 •

edited

Loading

neersighted commented Nov 15, 2021 •

edited

Loading