-
-
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
Type sequence checks in setuptools/dist.py #4578
Merged
abravalheri
merged 3 commits into
pypa:main
from
Avasam:type-setuptools-dist-sequence-methods
Oct 16, 2024
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix a `TypeError` when a ``Distribution``'s old included attribute was a `tuple` -- by :user:`Avasam` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Made errors when parsing ``Distribution`` data more explicit about the expected type (``tuple[str, ...] | list[str]``) -- by :user:`Avasam` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,16 @@ | |
import sys | ||
from glob import iglob | ||
from pathlib import Path | ||
from typing import TYPE_CHECKING, MutableMapping | ||
from typing import ( | ||
TYPE_CHECKING, | ||
Any, | ||
List, | ||
MutableMapping, | ||
NoReturn, | ||
Tuple, | ||
Union, | ||
overload, | ||
) | ||
|
||
from more_itertools import partition, unique_everseen | ||
from packaging.markers import InvalidMarker, Marker | ||
|
@@ -21,6 +30,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 | ||
|
@@ -36,9 +46,35 @@ | |
from distutils.fancy_getopt import translate_longopt | ||
from distutils.util import strtobool | ||
|
||
if TYPE_CHECKING: | ||
from typing_extensions import TypeAlias | ||
|
||
__all__ = ['Distribution'] | ||
|
||
sequence = tuple, list | ||
_sequence = tuple, list | ||
""" | ||
:meta private: | ||
|
||
Supported iterable types that are known to be: | ||
- ordered (which `set` isn't) | ||
- not match a str (which `Sequence[str]` does) | ||
- not imply a nested type (like `dict`) | ||
for use with `isinstance`. | ||
""" | ||
_Sequence: TypeAlias = Union[Tuple[str, ...], List[str]] | ||
# This is how stringifying _Sequence would look in Python 3.10 | ||
_requence_type_repr = "tuple[str, ...] | list[str]" | ||
|
||
|
||
def __getattr__(name: str) -> Any: # pragma: no cover | ||
if name == "sequence": | ||
SetuptoolsDeprecationWarning.emit( | ||
"`setuptools.dist.sequence` is an internal implementation detail.", | ||
"Please define your own `sequence = tuple, list` instead.", | ||
due_date=(2025, 8, 28), # Originally added on 2024-08-27 | ||
) | ||
return _sequence | ||
raise AttributeError(f"module {__name__!r} has no attribute {name!r}") | ||
|
||
|
||
def check_importable(dist, attr, value): | ||
|
@@ -51,17 +87,17 @@ def check_importable(dist, attr, value): | |
) from e | ||
|
||
|
||
def assert_string_list(dist, attr, value): | ||
def assert_string_list(dist, attr: str, value: _Sequence) -> None: | ||
"""Verify that value is a string list""" | ||
try: | ||
# verify that value is a list or tuple to exclude unordered | ||
# or single-use iterables | ||
assert isinstance(value, sequence) | ||
assert isinstance(value, _sequence) | ||
# verify that elements of value are strings | ||
assert ''.join(value) != value | ||
except (TypeError, ValueError, AttributeError, AssertionError) as e: | ||
raise DistutilsSetupError( | ||
"%r must be a list of strings (got %r)" % (attr, value) | ||
f"{attr!r} must be of type <{_requence_type_repr}> (got {value!r})" | ||
) from e | ||
|
||
|
||
|
@@ -126,8 +162,7 @@ def _check_marker(marker): | |
def assert_bool(dist, attr, value): | ||
"""Verify that value is True, False, 0, or 1""" | ||
if bool(value) != value: | ||
tmpl = "{attr!r} must be a boolean value (got {value!r})" | ||
raise DistutilsSetupError(tmpl.format(attr=attr, value=value)) | ||
raise DistutilsSetupError(f"{attr!r} must be a boolean value (got {value!r})") | ||
|
||
|
||
def invalid_unless_false(dist, attr, value): | ||
|
@@ -138,27 +173,31 @@ 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 | dict) -> NoReturn: ... | ||
@overload | ||
def check_requirements(dist, attr: str, value: _StrOrIter) -> None: ... | ||
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)): | ||
raise TypeError("Unordered types are not allowed") | ||
except (TypeError, ValueError) as error: | ||
tmpl = ( | ||
"{attr!r} must be a string or list of strings " | ||
"containing valid project/version requirement specifiers; {error}" | ||
msg = ( | ||
f"{attr!r} must be a string or iterable of strings " | ||
f"containing valid project/version requirement specifiers; {error}" | ||
) | ||
raise DistutilsSetupError(tmpl.format(attr=attr, error=error)) from error | ||
raise DistutilsSetupError(msg) from error | ||
|
||
|
||
def check_specifier(dist, attr, value): | ||
"""Verify that value is a valid version specifier""" | ||
try: | ||
SpecifierSet(value) | ||
except (InvalidSpecifier, AttributeError) as error: | ||
tmpl = "{attr!r} must be a string containing valid version specifiers; {error}" | ||
raise DistutilsSetupError(tmpl.format(attr=attr, error=error)) from error | ||
msg = f"{attr!r} must be a string containing valid version specifiers; {error}" | ||
raise DistutilsSetupError(msg) from error | ||
|
||
|
||
def check_entry_points(dist, attr, value): | ||
|
@@ -767,41 +806,43 @@ def has_contents_for(self, package): | |
|
||
return False | ||
|
||
def _exclude_misc(self, name, value): | ||
def _exclude_misc(self, name: str, value: _Sequence) -> None: | ||
"""Handle 'exclude()' for list/tuple attrs without a special handler""" | ||
if not isinstance(value, sequence): | ||
if not isinstance(value, _sequence): | ||
raise DistutilsSetupError( | ||
"%s: setting must be a list or tuple (%r)" % (name, value) | ||
f"{name}: setting must be of type <{_requence_type_repr}> (got {value!r})" | ||
) | ||
try: | ||
old = getattr(self, name) | ||
except AttributeError as e: | ||
raise DistutilsSetupError("%s: No such distribution setting" % name) from e | ||
if old is not None and not isinstance(old, sequence): | ||
if old is not None and not isinstance(old, _sequence): | ||
raise DistutilsSetupError( | ||
name + ": this setting cannot be changed via include/exclude" | ||
) | ||
elif old: | ||
setattr(self, name, [item for item in old if item not in value]) | ||
|
||
def _include_misc(self, name, value): | ||
def _include_misc(self, name: str, value: _Sequence) -> None: | ||
"""Handle 'include()' for list/tuple attrs without a special handler""" | ||
|
||
if not isinstance(value, sequence): | ||
raise DistutilsSetupError("%s: setting must be a list (%r)" % (name, value)) | ||
if not isinstance(value, _sequence): | ||
raise DistutilsSetupError( | ||
f"{name}: setting must be of type <{_requence_type_repr}> (got {value!r})" | ||
) | ||
try: | ||
old = getattr(self, name) | ||
except AttributeError as e: | ||
raise DistutilsSetupError("%s: No such distribution setting" % name) from e | ||
if old is None: | ||
setattr(self, name, value) | ||
elif not isinstance(old, sequence): | ||
elif not isinstance(old, _sequence): | ||
raise DistutilsSetupError( | ||
name + ": this setting cannot be changed via include/exclude" | ||
) | ||
else: | ||
new = [item for item in value if item not in old] | ||
setattr(self, name, old + new) | ||
setattr(self, name, list(old) + new) | ||
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. By adding annotations to the method, mypy now raises this issue. This fixes a |
||
|
||
def exclude(self, **attrs): | ||
"""Remove items from distribution that are named in keyword arguments | ||
|
@@ -826,10 +867,10 @@ def exclude(self, **attrs): | |
else: | ||
self._exclude_misc(k, v) | ||
|
||
def _exclude_packages(self, packages): | ||
if not isinstance(packages, sequence): | ||
def _exclude_packages(self, packages: _Sequence) -> None: | ||
if not isinstance(packages, _sequence): | ||
raise DistutilsSetupError( | ||
"packages: setting must be a list or tuple (%r)" % (packages,) | ||
f"packages: setting must be of type <{_requence_type_repr}> (got {packages!r})" | ||
) | ||
list(map(self.exclude_package, packages)) | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@Avasam, how about this for the deprecation of
sequence
?After a quick search on github, I do see people accessing it, so we cannot simply remove 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 had opted out of wanting to replace
sequence
bytyping.Sequence
in #4575 (comment) because there's a lot of implications when it comes to new checks and supported types (like having to explicitely check forstr
first everytime, because astr
is a validSequence[str]
, both at runtime and in type-checking).Not to say it wouldn't be an improvement, just that's not something I wanna go into for the scope of those PRs.
Is making
sequence
private something you want to do independent of those changes ? If so, this commit is fine yeah.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 think so. It seems to be such a trivial implementation detail, I don't know why people are importing it from setuptools.
It is probably good to deprecate it now just in case at some point we decide to remove it.