Skip to content

Conversation

@ri-gilfanov
Copy link

@ri-gilfanov ri-gilfanov commented Sep 30, 2024

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

ghost commented Sep 30, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Sep 30, 2024

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

bedevere-app bot commented Sep 30, 2024

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.

Copy link
Member

@sobolevn sobolevn left a comment

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

bedevere-app bot commented Sep 30, 2024

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.

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

ri-gilfanov commented Sep 30, 2024

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

Copy link
Member

@sobolevn sobolevn left a comment

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

@ri-gilfanov ri-gilfanov force-pushed the gh-124748 branch 5 times, most recently from 5e22f1c to 6125080 Compare September 30, 2024 21:05
@ri-gilfanov
Copy link
Author

ri-gilfanov commented Oct 1, 2024

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?

Copy link
Member

@sobolevn sobolevn left a comment

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

@ri-gilfanov
Copy link
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

@tomasr8
Copy link
Member

tomasr8 commented Oct 1, 2024

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 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants