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

Remove sprintf dependency #132

Merged
merged 3 commits into from
Nov 20, 2024
Merged

Remove sprintf dependency #132

merged 3 commits into from
Nov 20, 2024

Conversation

julemand101
Copy link
Contributor

Going though dependencies, it looks like the sprintf dependency are not really justified based on the functionality being used of this package. I therefore suggest removing the package as dependency to reduce the amount of maintenance burden of dart-uuid.

I am not entirely sure why this hex converting logic are made like it is since it does not really generate "real" hex values since 2024 are converted to 0x2024 and not 0x07E8 which would be the correct value when converting `2024´ into hex.

To prevent failing tests and potential breaking changes, my implementation will still generate the exact same hex-like values as before the sprintf removal. But if we are open for changing this design, I can easily change the logic so we generate the needed Uint8List lists directly from the int values instead of going though this hex conversions.

This info seems to be introduced with Dart 3.6 which is why it did not get reported before.
`UuidParsing.parseHexToBytes` can handle Strings without `0x` prefix.
@daegalus
Copy link
Owner

daegalus commented Nov 20, 2024

So, since this is a v8 uuid, it is very ambiguous and up to the implementer to create a design for it. This is just my example of it, hence why there is also a v8generic, that lets the user of the library design their own.

The goal of my v8 is that the date is human readable and still sortable. So if you generate a UUID on November 20th, 2024 at 13:42, the uuid will look like 20241120-1342-8ab0-9a7e-90ea1918.

It is completely random decision. I have no problem changing it to valid Hex. I don't think anyone is using v8 because its so specific and different. v8generic might be used, but not v8

I will merge this, as it is perfectly fine to fix it to not needs sprintf. I like reducing dependencies for a library like this. So thank you very much for your contribution.

If you wish to change the design, feel free to open a PR changing it.

@daegalus daegalus merged commit 77c3a33 into daegalus:main Nov 20, 2024
1 check passed
@julemand101
Copy link
Contributor Author

Thanks for merging.

If you wish to change the design, feel free to open a PR changing it.

I mean, if your design goal of your V8 implementation are to make those specific UUID's and there are no standard for it otherwise, I don't see much reason to introduce a breaking change to the format since there are really no issue in the implementation based on those goals.

So I don't expect to introduce any changes to this. But it is a nice reminder that UUIDv8 is something that should most likely not be used and the importance of actually look at what each UUID does. Higher number does not mean better standard here. :P

@daegalus
Copy link
Owner

Thanks for merging.

If you wish to change the design, feel free to open a PR changing it.

I mean, if your design goal of your V8 implementation are to make those specific UUID's and there are no standard for it otherwise, I don't see much reason to introduce a breaking change to the format since there are really no issue in the implementation based on those goals.

So I don't expect to introduce any changes to this. But it is a nice reminder that UUIDv8 is something that should most likely not be used and the importance of actually look at what each UUID does. Higher number does not mean better standard here. :P

Ya, v8 was designed to future proof the spec. As long as you set version 8 and a variant. Everything else is up to the implementer or user.

I recommend v7 for something consistent and standardized.

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.

2 participants