-
Notifications
You must be signed in to change notification settings - Fork 665
feat(uuid): stabilize uuidv7 module #6897
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6897 +/- ##
==========================================
+ Coverage 94.11% 94.12% +0.01%
==========================================
Files 582 582
Lines 42752 42763 +11
Branches 6808 6808
==========================================
+ Hits 40235 40250 +15
+ Misses 2466 2463 -3
+ Partials 51 50 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I should note what I didn't in my issue. The only change that I could see to this code is the type of the function returning a string. In typescript crypto.createUUID() returns a specific type that this does not. Wondering if it was ever discussed why this is the behavior std chose? |
|
I suppose at this point consistency is more important anyways and it would be weird to have one api have a different generate/validate typing. Maybe a 2.0 of this package could do that. |
kt3k
left a comment
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
uuid/mod.ts
Outdated
| import { validate as validateV4 } from "./v4.ts"; | ||
| import { generate as generateV5, validate as validateV5 } from "./v5.ts"; | ||
| import { | ||
| extractTimestamp, |
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.
a minor point for some consideration
I wonder if it's ok to export this name from here as all other top level methods have version suffixes.. What do you think about the name extractTimestampV7?
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.
This makes sense, I think. Other UUID types actually do store timestamps and we don't have functions to extract them right now.
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.
Wait sorry, I misread, we shouldn't do this. It's already on the v7 object that is being exported, we should do V7 on the import, in case in the future we decide to add more.
Closes #6772
Closes #6174
I'm a little bit rusty since I haven't contributed in a little bit. Didn't see docs in
CONTRIBUTING.mdfile explaining how stabilization works so I took my best shot at it. Let me know if there's anything else that needs to be done.