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.

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

bpo-17005: Add a topological sort algorithm #11583

Merged
merged 29 commits into from Jan 23, 2020
Merged

Conversation

@pablogsal
Copy link
Member

pablogsal commented Jan 17, 2019

@pablogsal pablogsal requested a review from rhettinger as a code owner Jan 17, 2019
@pablogsal pablogsal requested review from ericvsmith, tiran and abalkin Jan 17, 2019
@pablogsal pablogsal force-pushed the pablogsal:bpo17005 branch 6 times, most recently from 9048fbb to bc0512c Jan 17, 2019
Doc/library/functools.rst Outdated Show resolved Hide resolved
Copy link
Member

ericvsmith left a comment

I'm not sure if API design should happen here or back on the issue, but I'll start with here.

Doc/library/functools.rst Outdated Show resolved Hide resolved
@pablogsal pablogsal force-pushed the pablogsal:bpo17005 branch from bc0512c to 886086a Jan 17, 2019
Doc/library/functools.rst Outdated Show resolved Hide resolved
Doc/library/functools.rst Outdated Show resolved Hide resolved
Doc/library/functools.rst Outdated Show resolved Hide resolved
Doc/library/functools.rst Outdated Show resolved Hide resolved
Doc/library/functools.rst Outdated Show resolved Hide resolved
Doc/library/functools.rst Outdated Show resolved Hide resolved
Doc/library/functools.rst Outdated Show resolved Hide resolved
Lib/functools.py Outdated Show resolved Hide resolved
Lib/functools.py Outdated Show resolved Hide resolved
Lib/functools.py Outdated Show resolved Hide resolved
@pablogsal pablogsal force-pushed the pablogsal:bpo17005 branch from aafde00 to 28a9ff3 Jan 20, 2019
@pablogsal

This comment has been minimized.

Copy link
Member Author

pablogsal commented Feb 7, 2019

@rhettinger Could you advise about the API of this function? I think the new API is more convenient but a bit less powerful, but I am waiting for your input on this matter to continue development/discussion :)

Lib/functools.py Outdated Show resolved Hide resolved
@rhettinger rhettinger self-assigned this May 26, 2019
@pablogsal

This comment has been minimized.

Copy link
Member Author

pablogsal commented May 27, 2019

Just to make it simpler to evaluate. The equivalent input for:

['ABDGI', 'BEG', 'CEH', 'KCFHJ']

would be:

{'I': {'G'},
 'G': {'D', 'E'},
 'D': {'B'},
 'B': {'A'},
 'E': {'B', 'C'},
 'H': {'E', 'F'},
 'J': {'H'},
 'F': {'C'},
 'C': {'K'}}

and the result is:

[{'A', 'K'}, {'B', 'C'}, {'D', 'E', 'F'}, {'G', 'H'}, {'I', 'J'}]

representing that you can choose an arbitrary order in every group and that satisfies the sorting.

Lib/test/test_functools.py Outdated Show resolved Hide resolved
pablogsal added 2 commits Jan 18, 2020
@pablogsal pablogsal force-pushed the pablogsal:bpo17005 branch from 0406781 to 6229ad5 Jan 18, 2020
@pablogsal pablogsal force-pushed the pablogsal:bpo17005 branch from e90aaf1 to 618a791 Jan 18, 2020
Lib/functools.py Outdated Show resolved Hide resolved
Lib/test/test_functools.py Outdated Show resolved Hide resolved
pablogsal and others added 2 commits Jan 18, 2020
Co-Authored-By: Tim Peters <tim.peters@gmail.com>
Copy link
Contributor

sweeneyde left a comment

Minor spelling, mechanics, and clarity in docs.

Doc/library/functools.rst Outdated Show resolved Hide resolved
Doc/library/functools.rst Outdated Show resolved Hide resolved
Doc/library/functools.rst Outdated Show resolved Hide resolved
Doc/library/functools.rst Outdated Show resolved Hide resolved
Doc/library/functools.rst Outdated Show resolved Hide resolved
Doc/library/functools.rst Outdated Show resolved Hide resolved
Lib/functools.py Outdated Show resolved Hide resolved
Lib/functools.py Outdated Show resolved Hide resolved
Co-Authored-By: sweeneyde <36520290+sweeneyde@users.noreply.github.com>
Doc/library/functools.rst Show resolved Hide resolved
Lib/functools.py Outdated Show resolved Hide resolved
@pablogsal

This comment has been minimized.

Copy link
Member Author

pablogsal commented Jan 22, 2020

@tim-one @larryhastings The PR has been still for a while and I think is close to being ready for a first merge. I would like to get this in before the 3rd alpha so there is plenty of time to test this and still have possibilities to change things. Do you have any changes to the API that you would like to be changed? What about Larry's paint cans? 😆

Improvement to the docs can be made later as this can always be modified freely after the feature freeze :)

@pablogsal pablogsal requested review from tim-one and larryhastings Jan 22, 2020
Copy link
Member

tim-one left a comment

Ship it! 😃 I added one note, suggesting that CycleError be documented, and listing the salient points to cover.

Doc/library/functools.rst Show resolved Hide resolved
pablogsal added 2 commits Jan 23, 2020
@pablogsal pablogsal force-pushed the pablogsal:bpo17005 branch from 69ea6af to 89d07ce Jan 23, 2020
@pablogsal

This comment has been minimized.

Copy link
Member Author

pablogsal commented Jan 23, 2020

Now that the CycleError exception is now documented, I will go ahead and land this as I indicated in #11583 (comment). Let's open new, more focused PRs for documentation improvements, the bikeshedding stuff (looking at you @larryhastings 😉 ) and any other minor stuff. I will leave the issue open until we are completely happy with all aspects.

Thank you very much to everyone involved in this issue, the discussions and for all the patience with reviews and iterations (thanks to @tim-one especially 👌 ).

@pablogsal pablogsal merged commit 99e6c26 into python:master Jan 23, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200123.41 succeeded
Details
bedevere/issue-number Issue number 17005 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pablogsal pablogsal deleted the pablogsal:bpo17005 branch Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.