classification
Title: Improvements to graphlib.TopologicalSorter.static_order() documentation
Type: Stage: resolved
Components: Documentation Versions: Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: andrei.avk, bluetech, docs@python, miss-islington, pablogsal, rhettinger, tim.peters
Priority: normal Keywords: patch

Created on 2020-12-07 15:13 by bluetech, last changed 2021-06-29 14:19 by pablogsal. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 26834 merged andrei.avk, 2021-06-21 19:27
PR 26951 merged miss-islington, 2021-06-29 10:54
PR 26952 merged miss-islington, 2021-06-29 10:54
Messages (8)
msg382647 - (view) Author: Ran Benita (bluetech) Date: 2020-12-07 15:13
One issue and one suggestion.

Issue:

The documentation of prepare() says:

> If any cycle is detected, CycleError will be raised

which is what happens. The documentation of static_order() says that static_order() is equivalent to:

def static_order(self):
    self.prepare()
    while self.is_active():
        node_group = self.get_ready()
        yield from node_group
        self.done(*node_group)

specifically it is said to call self.prepare(), and also says

> If any cycle is detected, CycleError will be raised.

But, this only happens when the result of static_order is *iterated*, not when it's called, unlike what is suggested by the code and the comment.

Ideally, I think the call should raise the CycleError already if possible; this way, only the call can be wrapped in a try/except instead of the entire iteration. But if not, it should be clarified in the documentation.


Suggestion:

The documentation of static_order() says

> Returns an iterable of nodes in a topological order. Using this method does not require to call TopologicalSorter.prepare() or TopologicalSorter.done().

I think the wording "does not require" still implies that they *can* be called, but really they can't. If prepare() is called before static_order(), then when static_order() is iterated, "ValueError: cannot prepare() more than once" is raised.

I suggest this wording:

Returns an iterable of nodes in a topological order. When using this method, TopologicalSorter.prepare() and TopologicalSorter.done() should not be called.
msg382648 - (view) Author: Ran Benita (bluetech) Date: 2020-12-07 15:21
Hmm I realize after the fact that since the equivalent code snippet (which is actually the implementation) is a generator, then it is actually correct that prepare() isn't called until it is iterated. So the documentation for this is not incorrect.

Still, I think it would be better if the prepare() executes immediately rather than in the first iteration.
msg394188 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-05-22 22:08
Hi Ran, would you like to submit a pull request with the suggestion?
msg396282 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-06-21 19:33
I'm not sure it's worth it to complicate the logic by raising the error before iteration since iteration is normally done soon after method call.

I agree the doc change is helpful, since the ticket was dormant for a ~month, I've put up a PR with this change as well as clarifying that an iterator obj is returned rather than an iterable.

I've given credit to Ran in the PR description.
msg396711 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-29 10:54
New changeset 0d7f7975d55eff7e3dfcebd14e765fc6cd7d3e40 by andrei kulakov in branch 'main':
bpo-42588: Update the docs for the TopologicalSorter.static_order() method (GH-26834)
https://github.com/python/cpython/commit/0d7f7975d55eff7e3dfcebd14e765fc6cd7d3e40
msg396712 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-29 10:55
Thanks, Ran and Andrei for your contributions! :)
msg396713 - (view) Author: miss-islington (miss-islington) Date: 2021-06-29 11:14
New changeset d9fc4c3deb617beb1d0bbfdb4efc905a4192ff0a by Miss Islington (bot) in branch '3.10':
bpo-42588: Update the docs for the TopologicalSorter.static_order() method (GH-26834)
https://github.com/python/cpython/commit/d9fc4c3deb617beb1d0bbfdb4efc905a4192ff0a
msg396731 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-29 14:19
New changeset 3ba65cdcefcec7866be482138a5ea8aafbd07e59 by Miss Islington (bot) in branch '3.9':
bpo-42588: Update the docs for the TopologicalSorter.static_order() method (GH-26834) (GH-26952)
https://github.com/python/cpython/commit/3ba65cdcefcec7866be482138a5ea8aafbd07e59
History
Date User Action Args
2021-06-29 14:19:12pablogsalsetmessages: + msg396731
2021-06-29 11:14:55miss-islingtonsetnosy: + miss-islington
messages: + msg396713
2021-06-29 10:55:41pablogsalsetstatus: open -> closed

nosy: - miss-islington
messages: + msg396712

resolution: fixed
stage: patch review -> resolved
2021-06-29 10:54:55miss-islingtonsetpull_requests: + pull_request25520
2021-06-29 10:54:49miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request25519
2021-06-29 10:54:36pablogsalsetmessages: + msg396711
2021-06-21 19:33:35andrei.avksetmessages: + msg396282
2021-06-21 19:27:21andrei.avksetkeywords: + patch
nosy: + andrei.avk

pull_requests: + pull_request25415
stage: patch review
2021-05-22 22:08:46pablogsalsetmessages: + msg394188
2020-12-07 16:24:29xtreaksetnosy: + tim.peters, rhettinger, pablogsal
2020-12-07 15:21:00bluetechsetmessages: + msg382648
2020-12-07 15:13:10bluetechcreate