Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-110067: Make max heap methods public and add missing ones#130725

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
encukou merged 27 commits intopython:mainfromStanFromIreland:add-heapq-max
May 5, 2025

Conversation

StanFromIreland
Copy link
Contributor

@StanFromIrelandStanFromIreland commentedMar 1, 2025•
edited by github-actionsbot
Loading

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/

Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

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

There are various things to change. I don't know why we should expose the Python implementation and not the C implementation which is faster.

@bedevere-app
Copy link

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 phraseI 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.

@StanFromIreland
Copy link
ContributorAuthor

I was working on the C implementation, my bad!

@StanFromIreland
Copy link
ContributorAuthor

I have made the requested changes; please review again

@bedevere-app
Copy link

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

@bedevere-appbedevere-appbot requested a review frompicnixzMarch 1, 2025 15:40
Copy link
Member

@picnixzpicnixz left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I would group_max with their non-max variants and update their descriptions since theyare effictively different.

@bedevere-app
Copy link

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 phraseI 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.

@picnixzpicnixz marked this pull request as draftMarch 1, 2025 15:48
@picnixz
Copy link
Member

I'm converting into a draft until the comments are addressed. In addition, since we are now having code duplication forheappushpop_max andheappushpop, we could perhaps refactor the common logic (but maybe not in this PR yet)

@StanFromIreland
Copy link
ContributorAuthor

StanFromIreland commentedMar 1, 2025•
edited
Loading

I have made the requested changes; please review again

We can leave de-duplication for later.

@bedevere-app
Copy link

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

@bedevere-appbedevere-appbot requested a review frompicnixzMarch 1, 2025 16:11
StanFromIrelandand others added2 commitsMarch 17, 2025 16:05
Copy link
Contributor

@ambvambv left a comment

Choose a reason for hiding this comment

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

The issue your PR is solving is called "maxheap version of a heappush", but the PR is missing a C implementation of "heappush_max".

@StanFromIrelandStanFromIreland requested a review fromambvMay 2, 2025 14:56
@encukou
Copy link
Member

I put more suggestions inStanFromIreland#1.

encukouand others added2 commitsMay 5, 2025 14:27
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
max-heaps: Reword the docs, add more tests
Copy link
Member

@encukouencukou left a comment

Choose a reason for hiding this comment

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

One more editing pass, and let's get this into 3.14 beta.
There'll be time until July to adjust the tests/docs/implementation, or revert the feature entirely.

StanFromIreland reacted with hooray emoji
@encukouencukou merged commitf5b7847 intopython:mainMay 5, 2025
42 checks passed
@StanFromIrelandStanFromIreland deleted the add-heapq-max branchMay 5, 2025 16:05
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@picnixzpicnixzpicnixz left review comments

@encukouencukouencukou approved these changes

@rhettingerrhettingerAwaiting requested review from rhettingerrhettinger is a code owner

@ambvambvAwaiting requested review from ambv

Assignees

@rhettingerrhettinger

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@StanFromIreland@picnixz@rhettinger@encukou@ambv
[8]ページ先頭
©2009-2025 Movatter.jp