gh-130914: Make graphlib.TopologicalSorter.prepare() idempotent #131317
Conversation
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_nodesbut also te detect cycles. I overlooked that the latter should not be skipped. So raise an error at the startif self._npassedoutbut protect theself._ready_nodes = ...assignment with anif self._ready_nodes is Noneclause. -
The docs should change to make clear that
.prepare()must be called beforeget_readyO, but can be called multiple times before then (and even after ifget_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."
|
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 |
|
Thanks for making the requested changes! @tim-one: please review the changes made to this pull request. |
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.
|
|
||
| * 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`.) |
| 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.
|
OK, I kicked off another round of testing myself, and they all pass now. About to merge! |
…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/