Skip to content

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

Merged
rhettinger merged 3 commits into
python:masterfrom
pablogsal:heapq_argument_clinic
Sep 28, 2018
Merged

bpo-34797: Convert heapq to the argument clinic#9560
rhettinger merged 3 commits into
python:masterfrom
pablogsal:heapq_argument_clinic

Conversation

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

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

Maxheap variant of heappop
Copy link
Copy Markdown
Contributor

@taleinat taleinat Sep 26, 2018

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
Copy Markdown
Member Author

@pablogsal pablogsal Sep 26, 2018

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

bedevere-bot commented Sep 26, 2018

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
Copy Markdown
Member Author

pablogsal commented Sep 26, 2018

I have made the requested changes; please review again

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Sep 26, 2018

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

taleinat commented Sep 26, 2018

@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
Copy Markdown
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
Copy Markdown
Member Author

pablogsal commented Sep 26, 2018

@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
Copy Markdown
Contributor

rhettinger commented Sep 28, 2018

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