Skip to content

bpo-34797: Convert heapq to the argument clinic #9560

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 28, 2018
Merged

bpo-34797: Convert heapq to the argument clinic #9560

merged 3 commits into from
Sep 28, 2018

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Sep 25, 2018 •
edited by bedevere-bot
Loading

@pablogsal pablogsal self-assigned this Sep 25, 2018
serhiy-storchaka
serhiy-storchaka reviewed Sep 25, 2018
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Use more descriptive object instead of 'O'.

taleinat
taleinat requested changes Sep 26, 2018
View reviewed changes
Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

Looking good! Just one minor note.

Modules/_heapqmodule.c Outdated
heap: object
/

Maxheap variant of heappop
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the period at the end of the doc-string removed?

This applies here and in the other max-heap-variant functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch and thank you @taleinat !

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@pablogsal pablogsal force-pushed the heapq_argument_clinic branch from dabcdec to 277ac90 Compare September 26, 2018 12:54
@pablogsal
Copy link
Member Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@pablogsal pablogsal force-pushed the heapq_argument_clinic branch from 277ac90 to 7302181 Compare September 26, 2018 12:54
@taleinat
Copy link
Contributor

@pablogsal, please wait for @rhettinger's review before merging this in.

serhiy-storchaka
serhiy-storchaka approved these changes Sep 26, 2018
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM, but these changes need to be approved by @rhettinger.

@pablogsal
Copy link
Member Author

@taleinat @serhiy-storchaka Thanks for the review! I will wait for @rhettinger to approve this change.

@rhettinger rhettinger self-assigned this Sep 28, 2018
@rhettinger rhettinger merged commit e2f48bf into python:master Sep 28, 2018
@rhettinger
Copy link
Contributor

Nice work. Thanks.

@methane methane mentioned this pull request Apr 12, 2019
Merged
@pablogsal pablogsal deleted the heapq_argument_clinic branch May 19, 2021 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taleinat taleinat taleinat approved these changes

@rhettinger rhettinger rhettinger approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@auvipy auvipy auvipy approved these changes

@vstinner vstinner Awaiting requested review from vstinner

Assignees

@rhettinger rhettinger

@pablogsal pablogsal

Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pablogsal @bedevere-bot @taleinat @rhettinger @serhiy-storchaka @auvipy @the-knights-who-say-ni