bpo-35701: Added '__weakref__' to UUID __slots__ list to prevent regression. #11570
Conversation
…eferencing a UUID object
| @@ -657,6 +658,10 @@ def testIssue8621(self): | |||
|
|
|||
| self.assertNotEqual(parent_value, child_value) | |||
|
|
|||
| def test_uuid_weakref(self): | |||
There was a problem hiding this comment.
Yup, pushed the change
Lib/uuid.py
Outdated
| @@ -118,7 +118,7 @@ class UUID: | |||
| uuid_generate_time_safe(3). | |||
| """ | |||
|
|
|||
| __slots__ = ('int', 'is_safe') | |||
| __slots__ = 'int', 'is_safe', '__weakref__' | |||
There was a problem hiding this comment.
please keep parenthesis, IMHO they help for readability.
There was a problem hiding this comment.
I agree, have added these.
…on. Parantheses on __slots__ list
Lib/test/test_uuid.py
Outdated
| def test_uuid_weakref(self): | ||
| strong = self.uuid.uuid4() | ||
| weak = weakref.ref(strong) | ||
| assert strong is weak() |
There was a problem hiding this comment.
Please replace assert with self.assertIs().
There was a problem hiding this comment.
Of course, may I ask why?
There was a problem hiding this comment.
"assert" is removed when running python -O. Moreover, assertIs() provides better error messages on failure (render the two variables).
| @@ -0,0 +1 @@ | |||
| Re-enable the ability to weakly reference a UUID object. | |||
There was a problem hiding this comment.
It seems like https://bugs.python.org/issue30977 was never released, the change was only made in the master branch. So no user got the issue with a public release. If I'm right, the NEWS entry is useless.
There was a problem hiding this comment.
I believe you are correct, I was a bit unsure if I needed to then mention it. Should I drop the commit and force push? It seems like the bot detects when there is no news blurb and throws an error, is this optional then?
There was a problem hiding this comment.
Remove the NEWS entry. I will add the "skip news" label, don't worry.
Lib/test/test_uuid.py
Outdated
| @@ -657,6 +658,11 @@ def testIssue8621(self): | |||
|
|
|||
| self.assertNotEqual(parent_value, child_value) | |||
|
|
|||
| # bpo-35701: check that weak referencing to a UUID object can be created | |||
There was a problem hiding this comment.
nitpick: usually we put the comment inside the function, not outside ;-)
There was a problem hiding this comment.
no worries, I'll move it inside. Usually for something like this I would fixup the commit and force push, is that ok to do here or is the convention to use separate commits for the entire process?
There was a problem hiding this comment.
I prefer to have multiple commits, they will be squashed into a single commit as the end anyway.
There was a problem hiding this comment.
Different commits are simpler to review.
|
Thanks! I merged your PR. |
|
Thank you! There were some things regarding comments made in the "What's New" section brought up in the bpo issue, I will open a new PR for that once an agreement has been reached |
UUID objects don't currently allow for weak referencing with their new slots implementation. This would cause regressions if people using weak referencing on these objects were to upgrade Python version. Also added a basic regression test that fails without the change implemented.
This is my first PR to cpython, so please let me know if there is something I forgot to do!
https://bugs.python.org/issue35701