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

Using dict as an OrderedDict and allowed using dict as an ordered type in setuptools.dist.check_requirements #4575

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Using dict as an OrderedDict and allowed using dict as an ordered type in setuptools.dist.check_requirements #4575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions newsfragments/4575.feature.rst
View file
Open in desktop
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`
3 changes: 1 addition & 2 deletions setuptools/command/_requirestxt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,11 @@
from packaging.requirements import Requirement

from .. import _reqs
from .._reqs import _StrOrIter

# dict can work as an ordered set
_T = TypeVar("_T")
_Ordered = Dict[_T, None]
_ordered = dict
_StrOrIter = _reqs._StrOrIter


def _prepare(
Expand Down
5 changes: 1 addition & 4 deletions setuptools/command/egg_info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

Create a distribution's .egg-info directory and contents"""

import collections
import functools
import os
import re
Expand Down Expand Up @@ -211,11 +210,9 @@ def save_version_info(self, filename):
build tag. Install build keys in a deterministic order
to avoid arbitrary reordering on subsequent builds.
"""
egg_info = collections.OrderedDict()
# follow the order these keys would have been added
# when PYTHONHASHSEED=0
egg_info['tag_build'] = self.tags()
egg_info['tag_date'] = 0
egg_info = dict(tag_build=self.tags(), tag_date=0)
edit_config(filename, dict(egg_info=egg_info))

def finalize_options(self):
Expand Down
11 changes: 8 additions & 3 deletions setuptools/dist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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: ...
Comment on lines +142 to +143
Copy link
Member

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 prevent set. 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 their setup.py. Also, I'm unsure if there are other code paths. For example, can a user supply a set using pyproject.toml? It seems not.

So maybe the best thing would be to drop this override and suppress mypy errors/warnings about its usage.

Copy link
Contributor Author

@Avasam Avasam Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We don't want to be declaring that set is valid

That's what this overload is doing, preventing the use of set in check_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 it set 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 runtime typing_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 instance Sequence[str], which already matches a type of nested strings incidentally, and would prevent passing dict or set.

image

Copy link
Member

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.

Copy link
Contributor Author

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.

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.

@overload
def check_requirements(dist, attr: str, value: _StrOrIter) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

I think _StrOrIter is wrong here. An Iterable could be an Iterator that can be exhausted after the first _reqs.parse(). What we really want here is an OrderedSequence, which might be defined as every Sequence that's not unordered. I'm not sure if it's possible to define that in mypy, so an approximation is fine (maybe even _StrOrIter).

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):
Copy link
Contributor Author

@Avasam Avasam Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 set" rather than a "tuple or list". Is there any expectation to allow working with any iterable other than tuple or list? Basically the same question I had in #4575 (comment)

I also realize that whilst this is all technically type-safe, we may not want to allow dict for a user-facing reason: If a user is told they can use a dict here, they're likely going to expect that it means they can use a nested value of some sort, not a "dict with unused values". I also think that forcing a user to coerce their Iterable into a tuple or list in their config file so niche it should be fine.

Copy link
Member

Choose a reason for hiding this comment

The 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 Iterable or Sequence so as not to unnecessarily constrain the user. Probably only tuple or list are used commonly, but I'd be open to allowing users to pass in any ordered object with an __iter__.

In this case, we want to assert that whatever value was passed is ordered. The isinstance(, set) check is a poor approximation of that, and it's there mainly as a rough check that the user didn't make a silly mistake.

raise TypeError("Unordered types are not allowed")
except (TypeError, ValueError) as error:
tmpl = (
Expand Down
1 change: 0 additions & 1 deletion setuptools/tests/test_core_metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,6 @@ def test_parity_with_metadata_from_pypa_wheel(tmp_path):
python_requires=">=3.8",
install_requires="""
packaging==23.2
ordered-set==3.1.1
more-itertools==8.8.0; extra == "other"
jaraco.text==3.7.0
importlib-resources==5.10.2; python_version<"3.8"
Expand Down
10 changes: 2 additions & 8 deletions setuptools/tests/test_dist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import collections
import os
import re
import urllib.parse
Expand Down Expand Up @@ -72,15 +71,10 @@ def sdist_with_index(distname, version):


def test_provides_extras_deterministic_order():
extras = collections.OrderedDict()
extras['a'] = ['foo']
extras['b'] = ['bar']
attrs = dict(extras_require=extras)
attrs = dict(extras_require=dict(a=['foo'], b=['bar']))
dist = Distribution(attrs)
assert list(dist.metadata.provides_extras) == ['a', 'b']
attrs['extras_require'] = collections.OrderedDict(
reversed(list(attrs['extras_require'].items()))
)
attrs['extras_require'] = dict(reversed(attrs['extras_require'].items()))
dist = Distribution(attrs)
assert list(dist.metadata.provides_extras) == ['b', 'a']

Expand Down
Loading