Use dataclass to add type annotations to Extension so that it can easily be extended in setuptools.
#373
Conversation
|
There seems to be an unrelated error in I have rebased this PR on top of #374, so that we can see if the main log fails on not in the majority of the CI workers. Now the same unrelated errors as in #374 (comment) can be found. |
abravalheri
force-pushed
the
type-extension
branch
from
ae2a2cc to
37447a4
Compare
| # function) -- authors of new compiler interface classes are | ||
| # responsible for updating 'compiler_class'! | ||
| compiler_type: ClassVar[str] = None # type: ignore[assignment] | ||
| compiler_type: ClassVar[str] = None |
| def __repr__(self): | ||
| return f'<{self.__class__.__module__}.{self.__class__.__qualname__}({self.name!r}) at {id(self):#x}>' | ||
|
|
||
| # Legal keyword arguments for the Extension constructor |
There was a problem hiding this comment.
The phrase "Legal keyword arguments for the Extension constructor" was copied from core.py (not sure if "legal" is the best term to use).
|
I think that we could use the new |
|
CI errors seem to indicate formatting changes (possibly related to new versions of linters available). |
abravalheri
force-pushed
the
type-extension
branch
from
19a4a73 to
a24af92
Compare
Neat. Overall I like how it also brings the doc closer to its declarations (and deduplicates type definitions). Most of the added functional code is compat code that would later be removed.
There's a pile of different CI issues at the moment, I think we're gonna need them merged with careful review knowing the CI will stay red until all fixed:
|
| # Simple example | ||
| """ | ||
| from distutils.extension import Extension | ||
|
|
There was a problem hiding this comment.
why not just... have this code be directly in a file that mypy runs on ? You can use assert_type instead of reveal_type.
For example: https://github.com/python/typeshed/blob/main/stubs/setuptools/%40tests/test_cases/check_setup.py
You can even test for errors, as long as "no-unecessary type ignore" is enabled:
https://github.com/python/typeshed/blob/main/stubs/setuptools/%40tests/test_cases/check_protocols.py
There was a problem hiding this comment.
Mostly because I don't like the idea of manually writing the full signature of Extension.__init__ by hand, so that it can be added to the other side of the assertion/comparison :P
This way I can be lazy and just add some meaningful fragments.
And it does not matter much if mypy fails the typecheck due to other files in the project not being fully typed (right now I need to avoid the existing mypy.ini, otherwise reveal_type does not show anything)...
There was a problem hiding this comment.
Ah I glanced over the test too quickly and didn't see it was a dict, not a giant multiline string. Fair enough
|
I have also added |
abravalheri
force-pushed
the
type-extension
branch
from
acab60e to
7a083da
Compare
2 participants
In pypa/setuptools#5022, there is an effort to add type annotations to
setuptools.extension.Extension.However, right now it basically requires repeating all the
__init__arguments that are already specified indistutils.extension.Extensionand all the type annotations.If any argument changes (added/removed/changed type annotation), that needs to be repeated in
setuptools.There is a discussion in https://github.com/pypa/setuptools/pull/5022/files#r2124429494 about how would it be possible to avoid this duplication and need for manual synchronisation.
This PR is an attempt to use
dataclassfor that.I have experimented with other approaches (like attempting to use a TypedDict), but in the end of the day, this approach is what seems to me the most promising.
If I am not mistaken, this should allow backward compatibility. Type checkers are likely to start catching unknown keyword arguments, but I think that is a good thing.
On a related not, I did modify the warning to mention using unknown keywords is deprecated, the reasoning is that removing this lenient behaviour simplifies a lot of stuff and we can use dataclasses by the book.
The docstrings for the properties were basically moved from the existing class docstring.