-
-
Notifications
You must be signed in to change notification settings - Fork 30.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-89083: add support for UUID version 8 (RFC 9562) #123224
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change mostly LGTM, but there is missing documentation in uuid.rst ("TODO" ;-)).
By the way, |
I had a little interrogation here. Should we use keyword-only parameters (if so, which names? with/without the I also wondered whether to keep |
I'm fine with |
b = random.getrandbits(12) | ||
if c is None: | ||
import random | ||
c = random.getrandbits(62) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense / is it possible to reject values outside the expected value range? For example, reject negative numbers?
Maybe something like:
orig_a = a
a &= 0xffff_ffff_ffff
if a != orig_a: raise ValueError("...")
I don't know. Would it be consistent with other uuid functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UUIDv1 does not reject them so I wouldn't worry too much about it. v3 and v5 are not based on integral inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with having the same behavior than uuid1() in this case, since it's documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I would prefer to have a second core dev to review the feature.
(What's New conflict resolved) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Co-authored-by: Hugo van Kemenade <[email protected]>
Thanks Hugo for the review! I've changed the docstrings in tests to regular comments to avoid cluttering the output in verbose mode and used |
Merged, thanks @picnixz. |
Simplest implementation of UUIDv8. Extensible implementation where hash functions can be used may be a follow-up PR / feature.
Entropy considerations can be discussed directly on the PR since the issue is becoming larger and larger (but we could also consider opening a separate issue to discuss the implementation more in details).
📚 Documentation preview 📚: https://cpython-previews--123224.org.readthedocs.build/