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
Appearance settings

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

Merged
rhettinger merged 3 commits into
python:masterpython/cpython:masterfrom
pablogsal:heapq_argument_clinicpablogsal/cpython:heapq_argument_clinicCopy head branch name to clipboard
Sep 28, 2018
Merged

bpo-34797: Convert heapq to the argument clinic#9560
rhettinger merged 3 commits into
python:masterpython/cpython:masterfrom
pablogsal:heapq_argument_clinicpablogsal/cpython:heapq_argument_clinicCopy head branch name to clipboard

Conversation

@pablogsal

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

Copy link
Copy Markdown
Member

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

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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

@taleinat taleinat left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread Modules/_heapqmodule.c Outdated
heap: object
/

Maxheap variant of heappop

@taleinat taleinat Sep 26, 2018

Copy link
Copy Markdown
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.

@pablogsal pablogsal Sep 26, 2018

Copy link
Copy Markdown
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

bedevere-bot commented Sep 26, 2018

Copy link
Copy Markdown

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

pablogsal commented Sep 26, 2018

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

@bedevere-bot

bedevere-bot commented Sep 26, 2018

Copy link
Copy Markdown

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

taleinat commented Sep 26, 2018

Copy link
Copy Markdown
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

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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

pablogsal commented Sep 26, 2018

Copy link
Copy Markdown
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

rhettinger commented Sep 28, 2018

Copy link
Copy Markdown
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

@vstinner vstinner Awaiting requested review from vstinner

+1 more reviewer

@auvipy auvipy auvipy approved these changes

Reviewers whose approvals may not affect merge requirements

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.