-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Using dict
as an OrderedDict
and allowed using dict
as an ordered type in setuptools.dist.check_requirements
#4575
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Allowed using `dict` as an ordered type in ``setuptools.dist.check_requirements`` -- by :user:`Avasam` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ | |
import sys | ||
from glob import iglob | ||
from pathlib import Path | ||
from typing import TYPE_CHECKING, MutableMapping | ||
from typing import TYPE_CHECKING, MutableMapping, NoReturn, overload | ||
|
||
from more_itertools import partition, unique_everseen | ||
from packaging.markers import InvalidMarker, Marker | ||
|
@@ -21,6 +21,7 @@ | |
command as _, # noqa: F401 # imported for side-effects | ||
) | ||
from ._importlib import metadata | ||
from ._reqs import _StrOrIter | ||
from .config import pyprojecttoml, setupcfg | ||
from .discovery import ConfigDiscovery | ||
from .monkey import get_unpatched | ||
|
@@ -138,11 +139,15 @@ def invalid_unless_false(dist, attr, value): | |
raise DistutilsSetupError(f"{attr} is invalid.") | ||
|
||
|
||
def check_requirements(dist, attr, value): | ||
@overload | ||
def check_requirements(dist, attr: str, value: set) -> NoReturn: ... | ||
@overload | ||
def check_requirements(dist, attr: str, value: _StrOrIter) -> None: ... | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
def check_requirements(dist, attr: str, value: _StrOrIter) -> None: | ||
"""Verify that install_requires is a valid requirements list""" | ||
try: | ||
list(_reqs.parse(value)) | ||
if isinstance(value, (dict, set)): | ||
if isinstance(value, set): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it curious that this is the only function in this file checking for a "not
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, I try not to be overly-prescriptive about input types. For example, accept In this case, we want to assert that whatever value was passed is ordered. The |
||
raise TypeError("Unordered types are not allowed") | ||
except (TypeError, ValueError) as error: | ||
tmpl = ( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this at all. We don't want to be declaring that
set
is valid, because this function is specifically designed to preventset
. I would normally say let's just drop the imperative check and let users rely on mypy to check their types, but I'm guessing it's unlikely that anyone is type-checking theirsetup.py
. Also, I'm unsure if there are other code paths. For example, can a user supply a set usingpyproject.toml
? It seems not.So maybe the best thing would be to drop this override and suppress mypy errors/warnings about its usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what this overload is doing, preventing the use of
set
incheck_requirements
by indicating it'll never return (ie it'll always raise or exit the program).There's some cases where it doesn't help (like if
check_requirement
is the last statement of a block), but without itset
was already always valid anyway because it matches_StrOrIter
.One could also use the static
@deprecated
decorator on that overload to further re-enforce and message the user, but that's a bit cheesy (and wouldn't be clean as it'd require a runtimetyping_extension
import or alias)Finally as per https://github.com/pypa/setuptools/pull/4575/files#r1733535055 , we can instead try to restrict the type of
value
further. For instanceSequence[str]
, which already matches a type of nested strings incidentally, and would prevent passingdict
orset
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. As long as IDEs aren't tempted to suggest passing a
set
because of this annotation, I'm fine with it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just left home, but later I can take a look and show you what it looks like in VSCode/Pylance (especially when it comes to intellisense suggestions). As to not make assumptions here.