Skip to content

gh-124748: Fix handling kwargs in WeakKeyDictionary.update()#124783

Merged
kumaraditya303 merged 5 commits into
python:mainfrom
rigilfanov:gh-124748
Feb 18, 2026
Merged

gh-124748: Fix handling kwargs in WeakKeyDictionary.update()#124783
kumaraditya303 merged 5 commits into
python:mainfrom
rigilfanov:gh-124748

Conversation

@rigilfanov

@rigilfanov rigilfanov commented Sep 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Removed 2 lines for updating WeakKeyDictionary objects by keyword arguments. This did not work correctly because weak references to str objects are not supported.

Added clear exception that keyword arguments are not supported in update() method.

@ghost

ghost commented Sep 30, 2024 •
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app

bedevere-app Bot commented Sep 30, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app bedevere-app Bot mentioned this pull request Sep 30, 2024
Closed
@bedevere-app

bedevere-app Bot commented Sep 30, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

sobolevn
sobolevn reviewed Sep 30, 2024
View reviewed changes

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

Please, add:

  • a NEWS entry
  • a test case for this

Congrats on your first PR to CPython 👍

@bedevere-app

bedevere-app Bot commented Sep 30, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@rigilfanov rigilfanov force-pushed the gh-124748 branch 6 times, most recently from 10d0c4d to 51d4868 Compare September 30, 2024 16:44
@rigilfanov

rigilfanov commented Sep 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@sobolevn, thank you. Now all linters and checkers are green

sobolevn
sobolevn reviewed Sep 30, 2024
View reviewed changes

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

My take on better wording

Comment thread Misc/NEWS.d/next/Library/2024-09-30-15-31-59.gh-issue-124748.KYzYFp.rst Outdated
Comment thread Lib/weakref.py Outdated
@rigilfanov rigilfanov force-pushed the gh-124748 branch 5 times, most recently from 5e22f1c to 6125080 Compare September 30, 2024 21:05
tomasr8
tomasr8 reviewed Sep 30, 2024
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2024-09-30-15-31-59.gh-issue-124748.KYzYFp.rst Outdated
@rigilfanov

rigilfanov commented Oct 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author 
WARNING: py:meth reference target not found: weakref.WeakKeyDictionary.update [ref.meth]

@sobolevn and @tomasr8, I think the message in the Docs workflow is being output because the weakref module doesn't document the standard dictionary methods. Should I remove the reference to the undocumented method?

sobolevn
sobolevn approved these changes Oct 1, 2024
View reviewed changes

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

Other than my suggestion, this looks good to me.

But, I am not an expert in weakref. So, I would like to get a second review.

@rhettinger or @ambv, would you be interested? Based on https://github.com/python/cpython/commits/main/Lib/weakref.py

Comment thread Misc/NEWS.d/next/Library/2024-09-30-15-31-59.gh-issue-124748.KYzYFp.rst Outdated
@rigilfanov

rigilfanov commented Oct 1, 2024

Copy link
Copy Markdown
Contributor Author

@sobolevn, also when I fixed the message mismatch in the exception and test, I apparently undid 2 edits in it. I think I should put them back and do a squash of commits. There are not so many changes to keep a detailed history of them

@rigilfanov @sobolevn @tomasr8
pythongh-124748: Fix handling kwargs in WeakKeyDictionary.update()
564a09e 
Co-authored-by: sobolevn <mail@sobolevn.me>
Co-authored-by: Tomas R <tomas.roun8@gmail.com>
@tomasr8

tomasr8 commented Oct 1, 2024

Copy link
Copy Markdown
Member

I think I should put them back and do a squash of commits. There are not so many changes to keep a detailed history of them

Normally it's best to avoid squashing/force pushing even for small changes since it rewrites the history and can make reviewing the PR more difficult 🙂

sergey-miryanov
sergey-miryanov approved these changes Feb 17, 2026
View reviewed changes

@sergey-miryanov sergey-miryanov 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.

Code looks good to me.

kumaraditya303
kumaraditya303 reviewed Feb 17, 2026
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2024-09-30-15-31-59.gh-issue-124748.KYzYFp.rst Outdated
kumaraditya303
kumaraditya303 reviewed Feb 18, 2026
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2024-09-30-15-31-59.gh-issue-124748.KYzYFp.rst Outdated
@kumaraditya303 kumaraditya303 enabled auto-merge (squash) February 18, 2026 12:48
@kumaraditya303 kumaraditya303 merged commit 1636630 into python:main Feb 18, 2026
47 checks passed
brijkapadia pushed a commit to brijkapadia/cpython that referenced this pull request Feb 28, 2026
@rigilfanov @brijkapadia
pythongh-124748: Fix handling kwargs in WeakKeyDictionary.update() (p…
32e22ac 
…ython#124783)
ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
@rigilfanov @ljfp
pythongh-124748: Fix handling kwargs in WeakKeyDictionary.update() (p…
a7fab39 
…ython#124783)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomasr8 tomasr8 tomasr8 left review comments

@sergey-miryanov sergey-miryanov sergey-miryanov approved these changes

@sobolevn sobolevn sobolevn approved these changes

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rigilfanov @tomasr8 @sergey-miryanov @sobolevn @kumaraditya303