bpo-30977: rework code changes according to post-merge code review #9106
Conversation
Lib/test/test_uuid.py
Outdated
| for copy_func_name, copy_func in copiers: | ||
| with self.subTest(copy_func_name=copy_func_name): | ||
| for is_safe in self.uuid.SafeUUID: | ||
| u = self.uuid.UUID('d82579ce6642a0de7ddf490a7aec7aa5', |
There was a problem hiding this comment.
It can be created out of the loop for copy_func.
Lib/uuid.py
Outdated
| object.__setattr__(self, attr, value) | ||
| object.__setattr__(self, 'int', state['int']) | ||
| # is_safe was added in 3.7; it is also omitted when it is "unknown" | ||
| is_safe = SafeUUID(state.get('is_safe', SafeUUID.unknown.value)) |
There was a problem hiding this comment.
Would not be more clear (and efficient) if write it in the following form?
is_safe = SafeUUID(state['is_safe']) if 'is_safe' in state else SafeUUID.unknownOr
is_safe = SafeUUID(state.get('is_safe'))(since SafeUUID.unknown.value is None)?
There was a problem hiding this comment.
I'm not in favor of the second option because it breaks the SafeUUID enum abstraction.
I'll use the first.
Lib/test/test_uuid.py
Outdated
| for is_safe in self.uuid.SafeUUID: | ||
| u = self.uuid.UUID('d82579ce6642a0de7ddf490a7aec7aa5', | ||
| is_safe=is_safe) | ||
| self.assertEqual(u, copy_func(u)) |
There was a problem hiding this comment.
Needed to test is_safe.
Lib/test/test_uuid.py
Outdated
| for is_safe in self.uuid.SafeUUID: | ||
| u = self.uuid.UUID('d82579ce6642a0de7ddf490a7aec7aa5', | ||
| is_safe=is_safe) | ||
| self.assertEqual(u, copy_func(u)) |
There was a problem hiding this comment.
Usually in assertEqual() the first argument is an actual value, and the second argument is an expected value.
Lib/test/test_uuid.py
Outdated
|
|
||
| def test_pickle_roundtrip(self): | ||
| self._setup_for_pickle() | ||
| def pickle_roundtrip(obj, protocol): |
There was a problem hiding this comment.
This look cumbersome. I would write the test as:
def check(actual, expected):
self.assertEqual(actual, expected)
self.assertEqual(actual.is_safe, expected.is_safe)
with support.swap_item(sys.modules, 'uuid', self.uuid):
for is_safe in self.uuid.SafeUUID:
u = self.uuid.UUID('d82579ce6642a0de7ddf490a7aec7aa5',
is_safe=is_safe)
check(copy.copy(u), u)
check(copy.deepcopy(u), u)
for proto in range(pickle.HIGHEST_PROTOCOL + 1):
with self.subTest(protocol=proto):
check(pickle.loads(pickle.dumps(u, proto)), u)
Lib/test/test_uuid.py
Outdated
| b'4\x12xV4\x12xV4\x12xV4\x12sb.', | ||
| b'ccopy_reg\n_reconstructor\n(cuuid\nUUID\nc__builtin__\nobject\nNt' | ||
| b'R(dS\'int\'\nL287307832597519156748809049798316161701L\nsb.', | ||
| b'ccopy_reg\n_reconstructor\n(cuuid\nUUID\nc__builtin__\nobject\nNt' |
There was a problem hiding this comment.
It would look clearer if add comments between items. E.g.:
...
# Python 2.7, protocol 2
b'\x80\x02cuuid\nUUID\n)\x81}U\x03int\x8a\x11\xa5z\xecz\nI\xdf}\xde'
b'\xa0Bf\xcey%\xd8\x00sb.',
# Python 3.6, protocol 0
...
Lib/test/test_uuid.py
Outdated
| b'2171839636988778104L\nsb.', | ||
| b'\x80\x02cuuid\nUUID\nq\x00)\x81q\x01}q\x02U\x03intq\x03\x8a\x10xV' | ||
| b'4\x12xV4\x12xV4\x12xV4\x12sb.', | ||
| b'ccopy_reg\n_reconstructor\n(cuuid\nUUID\nc__builtin__\nobject\nNt' |
There was a problem hiding this comment.
The PEP 8 limit is 79 characters, not 80.
| b'\x80\x04\x95+\x00\x00\x00\x00\x00\x00\x00\x8c\x04uuid\x8c\x04UUID' | ||
| b'\x93)\x81}\x8c\x03int\x8a\x11\xa5z\xecz\nI\xdf}\xde\xa0Bf\xcey%' | ||
| b'\xd8\x00sb.', | ||
| ] |
There was a problem hiding this comment.
Just for the case add 3.7 pickles. This can help to prevent future regressions.
|
And since this change potentially breaks compatibility (UUID can't have other attributes and it is no longer weak referable), please add a What's New entry for it. |
Should we add |
|
@serhiy-storchaka, I've made the requested changes. Ready for another review! |
# Conflicts: # Doc/whatsnew/3.8.rst
|
I don't think anybody needs weak references to UUIDs. UUID objects are considered as atomic values, they should be small and immutable and should not referent other objects. Adding |
4 participants
This is a reworking of the changes made in PR #9078 according to the post-merge review by @serhiy-storchaka.
https://bugs.python.org/issue30977