gh-110067: Make max heap methods public and add missing ones #130725
Conversation
|
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 was working on the C implementation, my bad! |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
|
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'm converting into a draft until the comments are addressed. In addition, since we are now having code duplication for |
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
|
I have made the requested changes; please review again We can leave de-duplication for later. |
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
|
Also most of the requested changes can also be done for the existing code (I tried to stay consistent) should I open a second PR updating them to todays standards? |
I wouldn't for now. We should however wonder we can reduce the code duplication if possible. But this would need Raymond's approval. |
Misc/NEWS.d/next/Library/2025-03-01-15-00-00.gh-issue-110067.1ad3as.rst
Outdated
Show resolved
Hide resolved
Doc/whatsnew/3.14.rst
Outdated
| * Make :mod:`heapq` max-heap functions | ||
|
|
||
| * :func:`heapq.heapify_max`, | ||
| * :func:`heapq.heappush_max`, | ||
| * :func:`heapq.heappop_max`, | ||
| * :func:`heapq.heapreplace_max` | ||
|
|
||
| public. And add the missing :func:`heapq.heappushpop_max` to | ||
| both the C and Python implementation. |
There was a problem hiding this comment.
This paragraph needs some rewriting but I don't have time for now. But the idea is here.
There was a problem hiding this comment.
I rewrote it a bit nicer (at least in my opinion), is it better?
| @@ -0,0 +1,11 @@ | |||
| Make :mod:`heapq` max-heap functions | |||
There was a problem hiding this comment.
Blurb entries cannot have new lines. I'll suggest something else later the day.
There was a problem hiding this comment.
I removed new lines
|
@rhettinger, I would greatly appreciate your review :-) Currently this has been done:
Is there anything else left to do? coverage report
|
|
@rhettinger friendly ping so this doesn't get lost in your todo list :-) |
|
(Usually we wait 1 or 2 weeks for a friendly ping) |
|
I do want to review this but won't have time for a while. If there is a strong need to press forward (I'm not sure why), then another core-dev can take-over. Otherwise, it may have to sit for a little while. |
|
It seems to be quite a desired feature looking at the discuss and stack overflow. By while do you mean a week or two or a few months? |
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Made public, added test (same as normal heap) and added docs.
@rhettinger has not opened the planned PR in ~2 years
📚 Documentation preview 📚: https://cpython-previews--130725.org.readthedocs.build/