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

Avasam · 2024-08-17T22:46:35Z

Summary of changes

Allows using dict as an ordered type in setuptools.dist.check_requirements
#4574 Gave me the idea for this PR

Pull Request Checklist

Changes have tests (tests updated with new messages and dict)
News fragment added in newsfragments/.
(See documentation for details)

Avasam · 2024-08-17T23:06:07Z

setuptools/dist.py

@@ -52,17 +70,17 @@ def check_importable(dist, attr, value):
        ) from e


-def assert_string_list(dist, attr, value):
+def assert_string_list(dist, attr: str, value: _OrderedIterable):


This is more like assert_ordered_iterable now. 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.

I ended up reverting a lot of this, keeping strict tuple | list checks, and updating messages in #4578 instead.

Avasam · 2024-08-19T21:27:34Z

setuptools/dist.py

    """Verify that install_requires is a valid requirements list"""
    try:
        list(_reqs.parse(value))
-        if isinstance(value, (dict, set)):
+        if isinstance(value, set):


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.

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.

jaraco · 2024-08-27T21:00:46Z

setuptools/dist.py

    """Verify that install_requires is a valid requirements list"""
    try:
        list(_reqs.parse(value))
-        if isinstance(value, (dict, set)):
+        if isinstance(value, set):


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.

jaraco · 2024-08-27T21:04:43Z

setuptools/dist.py

+@overload
+def check_requirements(dist, attr: str, value: set) -> NoReturn: ...


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.

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.

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.

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.

Because of the necessary overload order, it still suggested sets first. Whilst it also showed it would never return NoReturn (return type), that didn't feel right. So I went with your other suggestion. I think that's good enough.

jaraco · 2024-08-27T21:16:23Z

setuptools/dist.py

+@overload
+def check_requirements(dist, attr: str, value: set) -> NoReturn: ...
+@overload
+def check_requirements(dist, attr: str, value: _StrOrIter) -> None: ...


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

…ed type in `setuptools.dist.check_requirements`

abravalheri · 2024-10-21T14:40:16Z

Thank you very much.

Avasam commented Aug 17, 2024

View reviewed changes

Avasam force-pushed the Allow-dict-as-ordered-set branch 2 times, most recently from e2d5654 to 0cfaff7 Compare August 18, 2024 05:04

Avasam changed the title ~~Allow using dict where an ordered iterable is expected~~ Allows using dict as an ordered type in setuptools.dist.check_requirements Aug 19, 2024

Avasam commented Aug 19, 2024

View reviewed changes

Avasam mentioned this pull request Aug 20, 2024

Type sequence checks in setuptools/dist.py #4578

Merged

2 tasks

Avasam force-pushed the Allow-dict-as-ordered-set branch from 8481ab4 to ee3a1d7 Compare August 20, 2024 17:12

Avasam changed the title ~~Allows using dict as an ordered type in setuptools.dist.check_requirements~~ Using dict as an OrderedDict and allowed using dict as an ordered type in setuptools.dist.check_requirements Aug 20, 2024

Avasam force-pushed the Allow-dict-as-ordered-set branch 2 times, most recently from a162b06 to 786e51a Compare August 27, 2024 15:00

jaraco approved these changes Aug 27, 2024

View reviewed changes

jaraco force-pushed the main branch from 809a9a7 to ffdf0bd Compare September 15, 2024 14:44

Avasam force-pushed the Allow-dict-as-ordered-set branch from 786e51a to ff57372 Compare October 17, 2024 15:55

Using dict as an OrderedDict and allowed using dict as an order…

2072d98

…ed type in `setuptools.dist.check_requirements`

Avasam force-pushed the Allow-dict-as-ordered-set branch from 38d20e8 to 2072d98 Compare October 17, 2024 16:20

abravalheri merged commit 31decc9 into pypa:main Oct 21, 2024
25 checks passed

Avasam deleted the Allow-dict-as-ordered-set branch October 21, 2024 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Avasam commented Aug 17, 2024 •

edited

Loading

Avasam Aug 17, 2024

Avasam Aug 17, 2024 •

edited

Loading

Avasam Aug 19, 2024 •

edited

Loading

jaraco Aug 27, 2024

jaraco Aug 27, 2024

jaraco Aug 27, 2024

Avasam Aug 28, 2024 •

edited

Loading

jaraco Aug 28, 2024

Avasam Aug 28, 2024

Avasam Oct 17, 2024

jaraco Aug 27, 2024

abravalheri commented Oct 21, 2024

		@overload
		def check_requirements(dist, attr: str, value: set) -> NoReturn: ...

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

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

Conversation

Avasam commented Aug 17, 2024 • edited Loading

Summary of changes

Pull Request Checklist

Choose a reason for hiding this comment

Avasam Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

Avasam Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avasam Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abravalheri commented Oct 21, 2024

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

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

Avasam commented Aug 17, 2024 •

edited

Loading

Avasam Aug 17, 2024 •

edited

Loading

Avasam Aug 19, 2024 •

edited

Loading

Avasam Aug 28, 2024 •

edited

Loading