bpo-22807: Expose platform UUID generation safety information. #138
Conversation
| @@ -182,6 +196,7 @@ def __init__(self, hex=None, bytes=None, bytes_le=None, fields=None, | |||
| int &= ~(0xf000 << 64) | |||
| int |= version << 76 | |||
| self.__dict__['int'] = int | |||
| self.__dict__['is_safe'] = is_safe | |||
There was a problem hiding this comment.
Do you want to assert the type of is_safe ? There seem to extensive checking of the rest of the parameters.
Not that I imagine people creating UUID object in their own code, but is_safe feel also a lot like a Boolean.
There was a problem hiding this comment.
I'm not sure it's worth type checking is_safe. The other ones are checked to ensure sane, compliant UUIDs, but is_safe is just a flag with some extra information (which may or may not be helpful to an application). If someone wants to manually instantiate UUIDs with bogus is_safe flags, oh well.
Lib/uuid.py
Outdated
| @@ -104,7 +114,7 @@ class UUID(object): | |||
| """ | |||
|
|
|||
| def __init__(self, hex=None, bytes=None, bytes_le=None, fields=None, | |||
| int=None, version=None): | |||
| int=None, version=None, is_safe=SafeUUID.unknown): | |||
There was a problem hiding this comment.
I changed my mind on whether this should affect older versions of Python.
If so, you can also consider making it kwarg-only. As you are already using is as a kwarg. Not sure if that's useful, or worth it. It's may also be a question of taste, and leaving it as is seem perfectly fine.
There was a problem hiding this comment.
Thanks for the review! I did think about that, and it might indeed make sense, but it's a matter of style in this case, and I wasn't sure I wanted to introduce that to the API. But on thinking about it again now that you bring it up, I does make sense.
There was a problem hiding this comment.
Thanks for the review!
My pleasure !
I did think about that, and it might indeed make sense, but it's a matter of style in this case, and I wasn't sure I wanted to introduce that to the API. But on thinking about it again now that you bring it up, I does make sense.
Happy to be of some use :-)
Doc/library/uuid.rst
Outdated
| .. class:: UUID(hex=None, bytes=None, bytes_le=None, fields=None, int=None, version=None) | ||
| .. class:: SafeUUID | ||
|
|
||
| .. attribute:: SafeUUID.safe |
There was a problem hiding this comment.
I know this is the current style in uuid.rst, but it might be better use the following style in new documentation:
.. class:: SafeUUID
.. versionadded:: 3.7
.. attribute:: safe
The UUID was generated by the platform in a multiprocessing-safe way.
...There was a problem hiding this comment.
Yeah, that looks nice, thanks. The one problem is that SafeUUID still gets labeled as a class in the HTML. Okay, technically it is a class, but it would be nice to have an enum:: markup. I grep'd around in the source and couldn't find any such examples, so I don't know if we support that yet.
There was a problem hiding this comment.
You're right. We don't have an enum directive now, but it should be easy to add one. You can add me to nosy list if you create an issue on bugs.p.o :)
Lib/test/test_uuid.py
Outdated
| @@ -340,6 +340,46 @@ def test_uuid1(self): | |||
| equal(((u.clock_seq_hi_variant & 0x3f) << 8) | | |||
| u.clock_seq_low, 0x3fff) | |||
|
|
|||
| @unittest.skipUnless(importable('ctypes'), 'requires ctypes') | |||
There was a problem hiding this comment.
Nitpick: I'd flip the order of the decorators.
| # unknown (unless I suppose the platform is buggy). | ||
| self.assertNotEqual(u.is_safe, uuid.SafeUUID.unknown) | ||
|
|
||
| @unittest.skipUnless(importable('ctypes'), 'requires ctypes') |
There was a problem hiding this comment.
Just a suggestion: Using self.subTest() would make the following four tests much shorter and IMO easier to understand.
There was a problem hiding this comment.
I've played a bit with subTest() and I'm not entirely thrilled with it. AFAICT, subtests only print output when they fail, not when they succeed. In verbose mode, I'd want to see all 4 tests printed individually. (That's one of the things I'd like to eventually fix.) For now, I think I'll leave them as separate tests.
Doc/library/uuid.rst
Outdated
|
|
||
| .. versionadded:: 3.7 | ||
|
|
||
| .. class:: UUID(hex=None, bytes=None, bytes_le=None, fields=None, int=None, version=None, *, is_safe=SafeUUID.unknown) |
There was a problem hiding this comment.
Currently, the body of UUID() documentation briefly describes all its arguments so I think we can just copy is_safe documentation in Lib/uuid.py
``local.getlocale(locale.LC_CTYPE)`` and ``locale.getpreferredencoding(False)`` may give different answers in some cases (such as the ``en_IN`` locale). ``re.LOCALE`` uses the latter, so update the test case to match.
Replace STACKLESS_PROPOSE(func) in do_call() and ext_do_call() by STACKLESS_PROPOSE_ALL().
- Initialize variable 'len' in function unwrap_frame_arg(). - Assign to '*valid' in function slp_find_execname(). - Disable GCC warning -Waddress in macro TASKLET_SETVAL(task, val). - Update Stackless/changelog.txt for issues python#138, python#140
Replace STACKLESS_PROPOSE(func) in do_call() and ext_do_call() by STACKLESS_PROPOSE_ALL(). (cherry picked from commit ae3d7a2)
- Assign to '*valid' in function slp_find_execname(). - Disable GCC warning -Waddress in macro TASKLET_SETVAL(task, val). - Update Stackless/changelog.txt for issues python#138, python#140 (cherry picked from commit 3461ccf)
Add an enum
SafeUUIDwhich describes whether the platform has generated a UUID in a safe manner (i.e. ensuring that no two processes can generate the same UUID). Also addUUID.is_safeattribute which relays the information from the underlying platform.