Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Correct types for setuptools.setup #12791

Merged
merged 7 commits into from
Oct 13, 2024
Merged

Correct types for setuptools.setup #12791

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7ab121e
Correct types for setuptools.setup
max-muoto Oct 12, 2024
1e6ccf1
Tweak
max-muoto Oct 12, 2024
f0d929f
Address feedback
max-muoto Oct 12, 2024
b1d1efb
Update stubs/setuptools/setuptools/__init__.pyi
max-muoto Oct 12, 2024
2bb4ed4
Address feedback on tuple
max-muoto Oct 13, 2024
82ad6cb
Fix
max-muoto Oct 13, 2024
7810a1e
Merge branch 'main' into correct-setuptools-types
max-muoto Oct 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions stubs/setuptools/setuptools/__init__.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
from _typeshed import Incomplete, StrPath
from abc import abstractmethod
from collections.abc import Iterable, Mapping, Sequence
from typing import Any, Literal, TypeVar, overload
from typing import Any, Literal, TypedDict, TypeVar, overload, type_check_only
from typing_extensions import NotRequired

from ._distutils.cmd import Command as _Command
from .command.alias import alias
Expand Down Expand Up @@ -45,6 +46,14 @@ __all__ = [

__version__: str

@type_check_only
class _BuildInfo(TypedDict):
sources: list[str] | tuple[str]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this isn't tuple[str, ...]? In other words, is this really always a one-tuple? (A few more times below.)

Copy link
Sponsor Collaborator

@Avasam Avasam Oct 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I also glanced over that...
(same for all definitions below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one tuple for macros is correct, but the others were a mistake: 2bb4ed4

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]]
Copy link
Sponsor Collaborator

@Avasam Avasam Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ^^)


# Pytype fails with the following:
# find_packages = PackageFinder.find
# find_namespace_packages = PEP420PackageFinder.find
Expand Down Expand Up @@ -85,7 +94,8 @@ def setup(
command_options: Mapping[str, Mapping[str, tuple[Incomplete, Incomplete]]] = ...,
package_data: Mapping[str, list[str]] = ...,
include_package_data: bool = ...,
libraries: list[str] = ...,
# libraries for `Distribution` or `build_clib`, not `Extension`, `build_ext` or `CCompiler`
libraries: list[tuple[str, _BuildInfo]] = ...,
max-muoto marked this conversation as resolved.
Show resolved Hide resolved
headers: list[str] = ...,
ext_package: str = ...,
include_dirs: list[str] = ...,
Expand Down