Correct types for setuptools.setup#12791
Conversation
This comment has been minimized.
This comment has been minimized.
|
|
||
| __version__: str | ||
|
|
||
| class _Library(TypedDict): |
There was a problem hiding this comment.
Should this be marked @type_check_only? There seems to be some precedence for using it on some similar typeddicts, e.g.
Lines 5 to 6 in 7ccc587
Also, to bikeshed the name, I see setuptools internally calls this structure a build_info in a few places, so maybe _BuildInfo would be a good fit?
There was a problem hiding this comment.
Interesting, I wasn't aware of it, but likely what we want here. Thanks for the callout. Agree that's a better name here.
This comment has been minimized.
This comment has been minimized.
| @type_check_only | ||
| class _BuildInfo(TypedDict): | ||
| sources: list[str] | tuple[str] | ||
| obj_deps: NotRequired[dict[str, list[str] | tuple[str]]] | ||
| macros: NotRequired[list[tuple[str, str] | tuple[str]]] | ||
| include_dirs: NotRequired[list[str]] | ||
| cflags: NotRequired[list[str]] |
There was a problem hiding this comment.
Normally I'd prefer defining such types close to where their implementation stands (so with the appropriate Command or Distribution) but in this case it'd end up in distutils and/or setuptools/_distutils so I guess in setuptools.__init__.py is fine.
I do have a concern using a type-only TypedDict with required members as a parameter: It makes it annoying to use a non-literal dict. Maybe that's ok in setuptools due to the nature of it looking like a config file, it shouldn't be much more complex than my example below?
It's possible, but requires a private type-only import, which isn't great:
from typing import cast, TYPE_CHECKING
if TYPE_CHECKING:
from setuptools import _BuildInfo
libs = [
("lib1", {"sources": []}),
("lib2", {"sources": []}),
]
setup(libraries=libs) # Type error in both mypy and pyright due to missing "sources"
# Ok, but must quote type to avoid runtime error
setup(libraries=cast(list[tuple[str, "_BuildInfo"]], libs))
# Ok
setup(libraries=[("literal", {"sources": []})])
# Ok
libs_annotated: list[tuple[str, "_BuildInfo"]] = [ # Must quote type or use from __future__ import annotations
("lib1", {"sources": []}),
("lib2", {"sources": []}),
]
setup(libraries=libs_annotated)There was a problem hiding this comment.
Yeah, perhaps it's not ideal but I'm not seeing a great alternative, obviously passing things in-line solves this problem, or just:
libs: "list[tuple[str, _BuildInfo]]" = [
("lib1", {"sources": []}),
("lib2", {"sources": []}),
]
setup(libraries=libs)Which would work without quotes given deferred annotations.
There was a problem hiding this comment.
At the same time, the kind of people to type-check their setup.py are probably the same going all out on typing (looking at myself here ^^)
Co-authored-by: Avasam <[email protected]>
This comment has been minimized.
This comment has been minimized.
|
|
||
| @type_check_only | ||
| class _BuildInfo(TypedDict): | ||
| sources: list[str] | tuple[str] |
There was a problem hiding this comment.
Are you sure this isn't tuple[str, ...]? In other words, is this really always a one-tuple? (A few more times below.)
There was a problem hiding this comment.
You're right, I also glanced over that...
(same for all definitions below)
There was a problem hiding this comment.
The one tuple for macros is correct, but the others were a mistake: 2bb4ed4
Avasam
left a comment
There was a problem hiding this comment.
Oops, I also glanced over this: tuple[str] means a single-item tuple, we want tuple[str, ...]
https://github.com/python/typeshed/pull/12791/files#r1797961597
Yep, I assumed they were actually one-tuples from the original issue but it looks like |
Additionally the second value in the tuple can be Here's a good example of what the types are allowed to be at runtime. |
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
this is necessary to have python/typeshed#12791
this is necessary to have python/typeshed#12791
this is necessary to have python/typeshed#12791
Fixes #12752