gh-130914: Make graphlib.TopologicalSorter.prepare() idempotent - #131317
Conversation
Sorry, something went wrong.
tim-one
left a comment
There was a problem hiding this comment.
Good start! Some changes needed:
-
Please add your name to Misc/ACKS (I don't see it there already).
-
prepare() has two jobs, to create ._ready_nodes but also te detect cycles. I overlooked that the latter should not be skipped. So raise an error at the start if self._npassedout but protect the self._ready_nodes = ... assignment with an if self._ready_nodes is None clause.
-
The docs should change to make clear that .prepare() must be called before get_readyO, but can be called multiple times before then (and even after if get_ready() returns an empty tuple - which it will do if the graph is empty, or if all nodes are in cycles).
-
We do need a "new in ..." box. One purpose of those is to help people writing code for multiple Python versions to avoid relying on new behaviors. This needn't be fancy. Something as simple as "prepare() before could be invoked at most once, although that wasn't fully documented."
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
|
Thanks for making the requested changes! @tim-one: please review the changes made to this pull request. |
Sorry, something went wrong.
tim-one
left a comment
There was a problem hiding this comment.
Looks good to me! Thank you 😄
I'll approve it, but can't merge it yet because tests are failing. Doesn't look to me that this has anything to with this patch, and expect it will "fix itself" shortly.
In the meantime, go to the bottom and click the button asking to merge the main branch into this PR. That should kick off another round ot testing.
Sorry, something went wrong.
picnixz
left a comment
There was a problem hiding this comment.
Small nits but otherwise LGTM.
Sorry, something went wrong.
|
|
||
| * Allow :meth:`graphlib.TopologicalSorter.prepare` to be called more than once | ||
| as long as sorting has not started. | ||
| (Contributed by Daniel Pope in :gh:`130914`) |
There was a problem hiding this comment.
| (Contributed by Daniel Pope in :gh:`130914`) | |
| (Contributed by Daniel Pope in :gh:`130914`.) |
Sorry, something went wrong.
| for attempt in range(1, 4): | ||
| with self.assertRaises(graphlib.CycleError, msg=f"{attempt=}"): | ||
| ts.prepare() |
There was a problem hiding this comment.
| for attempt in range(1, 4): | |
| with self.assertRaises(graphlib.CycleError, msg=f"{attempt=}"): | |
| ts.prepare() | |
| for attempt in range(1, 4): | |
| with self.subTest(attempt=attempt): | |
| self.assertRaises(graphlib.CycleError, ts.prepare) |
You can also use subTest() if you want.
Sorry, something went wrong.
|
OK, I kicked off another round of testing myself, and they all pass now. About to merge! |
Sorry, something went wrong.
…empotent (python#131317)" This reverts commit c1b42db.
…python#131317) Closes python#130914: Make graphlib.TopologicalSorter.prepare() idempotent Relax the rules so that `.prepare()` can be called multiple times, provided that no work has been passed out by `.get_ready()` yet.
Allow calling .prepare() more than once as long as ._npassedout is 0.
Add tests, blurb and what's new.
The graphlib documentation does not mention that .prepare() may not be callled more than once and therefore does not need to be updated.
Fixes #130914
📚 Documentation preview 📚: https://cpython-previews--131317.org.readthedocs.build/