GeezerVison

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@ruaridhw

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Found these docs a little misleading, please let me know if this requires a BPO entry.

The same can be said of combinations_with_replacement(), permutations(), and product() so if this is applicable, probably best to add changes to those docs as well.

I think we should keep the reference to the lexicographic order

@remilapeyre, what was your reasoning for this? Personally, this was the confusing part for me and the only reference that I'd like to remove. I think I'm missing something because I can't see how these methods care about lexicographic order at all.

Take for example, a completely unsortable class:

from itertools import combinations


class NotSortable:
    def __init__(self, id_):
        self.id = id_

    def __repr__(self):
        return f'{self.id}'


sorted([NotSortable(3), NotSortable(2)])
## TypeError: '<' not supported between instances of 'NotSortable' and 'NotSortable'
combs = list(combinations([NotSortable(3), NotSortable(2), NotSortable(4), NotSortable(1)], 2))
print(combs)
## [(3, 2), (3, 4), (3, 1), (2, 4), (2, 1), (4, 1)]

Surely any "lexicographic order" (or any ordering for that matter other than the one provided) of this input iterable is undefined?

With this unsortable class we can have the objects

a = NotSortable(3)
b = NotSortable(2)
c = NotSortable(4)
d = NotSortable(1)

and we can define pos() so that

pos(a) = 1
pos(b) = 2
pos(c) = 3
pos(d) = 4

and pos() defines an order over {a, b, c, d} and combinations() will output pairs in lexicographic order. It's just not the order defined by NotSortable.__lt__() but the one defined by list.index(). It can be confusing but it seems to me that it is correct and we should maybe improve the phrasing but not remove the whole paragraph.

Hmmm that does seem highly confusing. My point is that there are no cases where it output order is defined by anything other than list.index(). Hence we should explicitly state this.

Stating "combinations are emitted in lexicographic sort order" reads to me as if the elements of the iterable or the iterable itself must define a lexicographic sort order and this is not the case. Especially because if the elements do define a lexicographic sort order (eg. str), this is not the order of the output pairs:

>>> print(list(combinations('dbca', 2)))
## [('d', 'b'), ('d', 'c'), ('d', 'a'), ('b', 'c'), ('b', 'a'), ('c', 'a')]

@ruaridhw, please address the code review comments. Thank you!

@ruaridhw, thank you for making the change. I apologize for not seeing this before, but I just noticed you made this PR against the 3.8 branch instead of the master branch. Generally, all of our PRs are against master and then we have a bot that will backport changes to other releases. Based on that, please change this to be over the master branch. It's a little more involved than just changing the base branch. Thanks!

Attempting to restart the bots by closing and reopening.

Thanks @ruaridhw for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

GH-20501 is a backport of this pull request to the 3.9 branch.

GH-20502 is a backport of this pull request to the 3.8 branch.