Skip to content
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

Merged
merged 14 commits into from
Nov 12, 2024

Conversation

picnixz
Copy link
Contributor

@picnixz picnixz commented Aug 22, 2024

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/

Copy link
Member

@vstinner vstinner left a 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" ;-)).

Lib/uuid.py Outdated Show resolved Hide resolved
Lib/uuid.py Outdated Show resolved Hide resolved
Lib/test/test_uuid.py Show resolved Hide resolved
Doc/library/uuid.rst Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

vstinner commented Sep 19, 2024

By the way, uuid8(a=None, b=None, c=None): I was surprised by the "generic" argument names (a, b, c), but RFC 9562 uses the same names (custom_a, custom_b, custom_c): https://www.rfc-editor.org/rfc/rfc9562.html#name-uuid-version-8

@picnixz
Copy link
Contributor Author

picnixz commented Sep 19, 2024

I had a little interrogation here. Should we use keyword-only parameters (if so, which names? with/without the custom_ prefix?)?

I also wondered whether to keep custom_a or a but if people want to use keywords (without the parameters being keyword-only) the first one does not give more information so the short name is probably preferred to reduce the number of keystrokes...

@vstinner
Copy link
Member

I'm fine with uuid8(a=None, b=None, c=None) API: short parameter names (not custom_ prefix) and allow passing arguments as positional arguments.

b = random.getrandbits(12)
if c is None:
import random
c = random.getrandbits(62)
Copy link
Member

@vstinner vstinner Sep 26, 2024

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?

Copy link
Contributor Author

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.

Copy link
Member

@vstinner vstinner Sep 26, 2024

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.

Copy link
Member

@vstinner vstinner left a 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.

@hugovk
Copy link
Member

hugovk commented Nov 2, 2024

(What's New conflict resolved)

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Doc/library/uuid.rst Outdated Show resolved Hide resolved
Doc/library/uuid.rst Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <[email protected]>
Lib/test/test_uuid.py Outdated Show resolved Hide resolved
Doc/library/uuid.rst Outdated Show resolved Hide resolved
Doc/library/uuid.rst Outdated Show resolved Hide resolved
@picnixz
Copy link
Contributor Author

picnixz commented Nov 11, 2024

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 .. versionadded:: next.

@vstinner vstinner merged commit 03924b5 into python:main Nov 12, 2024
36 checks passed
@vstinner
Copy link
Member

Merged, thanks @picnixz.

@picnixz picnixz deleted the uuid-v8-89083 branch November 14, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants