Skip to content
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 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

bpo-47220: Document the optional callback parameter of weakref.WeakMethod #25491

Merged
merged 14 commits into from Nov 30, 2022
Merged

bpo-47220: Document the optional callback parameter of weakref.WeakMethod #25491

merged 14 commits into from Nov 30, 2022
+6 −2

Conversation

maggyero
Copy link
Contributor

@maggyero maggyero commented Apr 20, 2021 •
edited

This PR will bring the following changes to the weakref documentation:

  • document the optional callback parameter of weakref.WeakMethod;
  • normalise the use of ‘proxy’, replacing the mix of ‘proxy’ (8 occurrences) and ‘proxy object’ (3 occurrences);
  • normalise the use of ‘weak reference’, replacing the mix of ‘weak reference’ (37 occurrences), ‘weak reference object’ (7 occurrences), ‘reference’ (7 occurrences), ‘reference object’ (6 occurrences), ‘weakref’ (3 occurrences), and ‘weakref object’ (1 occurrence).

https://bugs.python.org/issue47220

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Apr 20, 2021
@maggyero maggyero marked this pull request as ready for review Apr 23, 2021
@github-actions
Copy link

github-actions bot commented Jun 3, 2021

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jun 3, 2021
@JelleZijlstra JelleZijlstra self-requested a review Apr 2, 2022
@JelleZijlstra
Copy link
Member

JelleZijlstra commented Apr 3, 2022

Sorry this had to wait for so long!

  • Documenting the callback parameter to WeakMethod requires an issue and a NEWS entry in my view. Also, the documentation should say what the parameter does.
  • Some of the wording changes feel gratuitous (e.g. "proxy objects" vs. "proxies"). I don't know if the original authors had reasons for using one form or the other, so I'd be hesitant to make changes without a strong reason.

@JelleZijlstra JelleZijlstra removed their request for review Apr 3, 2022
merwok
merwok reviewed Apr 3, 2022
View changes
Doc/library/weakref.rst Outdated Show resolved Hide resolved
@maggyero maggyero changed the title Update weakref.rst bpo-47220: Document the optional callback parameter of weakref.WeakMethod Apr 4, 2022
@maggyero maggyero changed the title bpo-47220: Document the optional callback parameter of weakref.WeakMethod Document the optional callback parameter of weakref.WeakMethod Apr 4, 2022
@maggyero maggyero changed the title Document the optional callback parameter of weakref.WeakMethod bpo-47220: Document the optional callback parameter of weakref.WeakMethod Apr 4, 2022
@maggyero
Copy link
Contributor Author

maggyero commented Apr 4, 2022 •
edited

@JelleZijlstra @merwok Thanks for the review.

  • Documenting the callback parameter to WeakMethod requires an issue and a NEWS entry in my view. Also, the documentation should say what the parameter does.

Done.

  • Some of the wording changes feel gratuitous (e.g. "proxy objects" vs. "proxies"). I don't know if the original authors had reasons for using one form or the other, so I'd be hesitant to make changes without a strong reason.

I normalised the use of ‘proxy’ because a mix of ‘proxy’ (8 occurrences) and ‘proxy object’ (3 occurrences) was used.

erlend-aasland
erlend-aasland requested changes Nov 27, 2022
View changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Can you please submit an update that just focuses on the core issue.

Resist the urge to rewrite everything you look at. Many of the edits in this and other PRs are gratuitous and/or dubious. Reviewing each one and thinking carefully about them eats a lot of reviewer time. Also, I concur with @merwok that some of these changes make the text harder to read. Some of this wording has been in existence for many years with no evidence of causing confusion with users. So, there is no real problem being solved.

+1

Please remove all unrelated changes.

@bedevere-bot
Copy link

bedevere-bot commented Nov 27, 2022

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

@maggyero
Copy link
Contributor Author

maggyero commented Nov 28, 2022

Please remove all unrelated changes.

Reverted. I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented Nov 28, 2022

Thanks for making the requested changes!

@erlend-aasland: please review the changes made to this pull request.

@erlend-aasland erlend-aasland requested a review from rhettinger Nov 28, 2022
@rhettinger rhettinger merged commit 9628136 into python:main Nov 30, 2022
14 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Nov 30, 2022

Thanks @maggyero for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Nov 30, 2022

Sorry @maggyero and @rhettinger, I had trouble checking out the 3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 9628136fac997847b4662e6a17faf06d2a0507eb 3.11

@miss-islington
Copy link
Contributor

miss-islington commented Nov 30, 2022

Thanks @maggyero for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

bedevere-bot commented Nov 30, 2022

GH-99909 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 30, 2022
@maggyero @miss-islington
bpo-47220: Document the optional callback parameter of weakref.WeakMe…
eda7e37 
…thod (pythonGH-25491)

(cherry picked from commit 9628136)

Co-authored-by: Géry Ogam <gery.ogam@gmail.com>
miss-islington added a commit that referenced this pull request Nov 30, 2022
@miss-islington @maggyero
bpo-47220: Document the optional callback parameter of weakref.WeakMe…
90701eb 
…thod (GH-25491)

(cherry picked from commit 9628136)

Co-authored-by: Géry Ogam <gery.ogam@gmail.com>
@maggyero
Copy link
Contributor Author

maggyero commented Nov 30, 2022

Thanks for the review!

@maggyero maggyero deleted the patch-25 branch Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merwok merwok

@rhettinger rhettinger

@pitrou pitrou

@erlend-aasland erlend-aasland

Assignees

@rhettinger rhettinger

Labels
docs Documentation in the Doc dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet