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

Inconsistencies in TypeID class #9

Closed
attevaltojarvi opened this issue Apr 10, 2024 · 2 comments
Closed

Inconsistencies in TypeID class #9

attevaltojarvi opened this issue Apr 10, 2024 · 2 comments

Comments

@attevaltojarvi
Copy link
Contributor

Hi, and thanks for this package!

I noticed a couple of inconsistencies in the TypeID class:

@classmethod
def from_uuid(cls, suffix: UUID, prefix: Optional[str] = None):
    suffix_str = _convert_uuid_to_b32(suffix)
    return TypeID(suffix=suffix_str, prefix=prefix)

this class method explicitly returns a TypeID, when it could use cls, like the from_string method right above it.

def _convert_b32_to_uuid(b32: str) -> UUID:
    return UUID(bytes=bytes(base32.decode(b32)))

I think this utility function should return the customized uuid6.UUID instead of uuid.UUID, since the class constructor calls uuid6.uuid7(). The difference is subtle, but this way the .uuid property can return incorrect results if you try to access .uuid.time:

uuid = uuid6.uuid7()
print(uuid.time)
>>> 1712728754957

tid = typeid.TypeID()
print(tid.uuid.time)
>>> 688659269629691542

uuid6.UUID has an overridden time property, which checks the version, and returns super().time if the version is not 6, 7, or 8.

The utility function could be updated like so:

def _convert_b32_to_uuid(b32: str):
    uuid_bytes = bytes(base32.decode(b32))
    uuid_int = int.from_bytes(uuid_bytes)
    return uuid6.UUID(int=uuid_int, version=7)

I ran into this while playing around with the idea of modifying the uuid6.uuid7 generator function to receive the timestamp part as a parameter, so that I could also create TypeID instances based on some historical timestamp. This'd be a really useful feature when migrating from database-generated sequenced numeric IDs to TypeIDs. Some other ID generation libraries support this.

I can open a PR for these changes if you'd like. Thanks again!

@akhundMurad
Copy link
Owner

Hello @attevaltojarvi!
I appreciate your interest in TypeID.
Waiting for your PR 😁

@akhundMurad
Copy link
Owner

resolved by #10

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

No branches or pull requests

2 participants