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 pablogsal commented Jan 17, 2019

Doc/library/functools.rst Outdated Show resolved Hide resolved
Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

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
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
Copy link
Member Author

@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
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/functools.py Outdated Show resolved Hide resolved
Lib/test/test_functools.py Outdated Show resolved Hide resolved
Copy link
Member

@sweeneyde sweeneyde left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

@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 :)

Copy link
Member

@tim-one tim-one left a comment

Choose a reason for hiding this comment

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

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
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
@pablogsal pablogsal deleted the bpo17005 branch January 23, 2020 15:29
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
… library (pythonGH-11583)

Co-Authored-By: Tim Peters <tim.peters@gmail.com>
@@ -166,6 +166,13 @@ ftplib
if the given timeout for their constructor is zero to prevent the creation of
Copy link
Member

Choose a reason for hiding this comment

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

@pablogsal the file Doc/myfile.bz2 above does not seem referenced from anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

That has been corrected since this PR :)

Copy link
Member Author

@pablogsal pablogsal Sep 16, 2021

Choose a reason for hiding this comment

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

Particulary in 67673b0 in #21286

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet