refactors Timestamp, Context and ClockSequence, and adds UUIDS v6, v7, v8 #611
No reviewers
Labels
No labels
breaking
bug
dependencies
good first issue
help wanted
unstable
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-uuid-rs/uuid!611
Loading…
Reference in a new issue
No description provided.
Delete branch "six-seven-eight"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I'm submitting a feature
Description
This addresses all requested features in #523
Adds v6, v7 and v8 features and modules.
It also adds a Timestamp::now() which does what you might expect and uses std::time::SystemTime.
I added some basic tests for each, but they could certainly use more tests and more eyes.
Breaking changes
This refactors Timestamp to make it more generally applicable to all (time based) UUIDs.
It also breaks context/sequence out of Timestamp, since that only applies to v1 and v6.
This changes the API for v1 and v6, to require a Context to be supplied at creation.
Updates the crate to Edition 2021
Dependency Changes
Removes a dependency on atomic and instead uses std::sync::atomic since it now supports the necessary types.
Related Issue(s)
#523
Just want to say from a lib maintainer's perspective, there's basically no good reason that such a widely used crate should need to get bumped to
edition = 2021. It hurts compatibility for folks on earlier compilers/editions far more than it helps maintainability (compared to 2015->2018 shift).@robjtede - Thanks for the input. The shift to 2021 really wasn't much more than changing the value in the Cargo.toml. What about it hurts compatibility?
The msrv for edition 2021 is 1.56.0;
mainis still targeting an msrv of 1.46.0.Thanks for working on this @rrichardson! 🙇
We can't make breaking changes to UUID's API now that it's stable, so I think our best option with the timestamp is to either:
Neither of these are as ideal as your refactoring, but I think we need to work within the API we've got.
@ -0,0 +72,4 @@/// NOTE: This function will panic if the current system time is earlier than/// the Unix Epoch of 1970-01-01 00:00:00 So please use caution when time travelling.#[cfg(all(feature = "std", not(any(feature = "v1", feature = "v6"))))]pub fn now() -> Self {This is a great change 👍
We can't really make breaking changes to UUID, so while this change is clearer than the current API I think we'll need to stick with what we've got.
OK.. I'll make this PR conform to the existing Library/Tests. I will make a follow up PR with an RFC for my proposed API refactorings re: https://github.com/uuid-rs/uuid/issues/612
@KodrAus - Do you know any ways that we can work around the fact that 2 of the CI jobs fail when running the powerset of features (runs the container out of space) I'm not a Github Actions wizard. It seems 15 features pushes it over the failing point.
@rrichardson I'll send a PR through that rejigs that part of CI a bit. The main thing we want to ensure is that each feature works independently, and that they all work together. I'll give you a ping when that's up.
@KodrAus - Ok.. This should be compatible with the original API. Note, this API requires some silly hoop-jumping for any versions that use a timestamp but not a counter (e.g. v7 https://github.com/rrichardson/uuid/blob/six-seven-eight/src/v7.rs#L53-L61)
All checks have passed! 🎊 🍾
This is awesome work! Thanks for putting it together, and for sticking through our current API constraints 🙇 I just had a few minor comments.
Was making this field public intentional?
@ -0,0 +1,233 @@/// Contains the `Timestamp` struct and `ClockSequence` traitsI think this can happen on systems with drifting clocks, maybe more likely to be observed on Windows since its epoch is earlier than the Unix one?
I don't have a great alternative for what we should do, besides documenting that this method will panic if the system time is set earlier than the Unix epoch.
@ -0,0 +1,233 @@/// Contains the `Timestamp` struct and `ClockSequence` traitsI think that clock drift shouldn't be a problem. The Result returned by
SystemTime::elapsed()is an error if self's time happens after now. In this case, it is checking that that the current system time is later than Jan 1st, 1970.I'll update the documentation and the message accordingly.
@ -0,0 +1,233 @@/// Contains the `Timestamp` struct and `ClockSequence` traitsYeh, so less drifting clocks, and maybe more likely misconfigured Windows systems, where their epoch is some much earlier date than the Unix one 🤔
Yeah.. this was necessary due to moving modules around. actually.. it could probably be pub (crate)
ok.. I just checked and maybe it's not necessary. Removed.
@ -0,0 +1,167 @@//! The implementation for Version 1 UUIDs.I think you meant to say
v6feature@ -0,0 +1,91 @@//! The implementation for Version 1 UUIDs.Same here
@ -0,0 +1,167 @@//! The implementation for Version 1 UUIDs.Righto. Thanks!
BTW: Arbitrary just bumped their patch version which added the use of array::from_fn which was just stabilized in Rust 1.63. So some of the CI jobs failed because of this. I've locked Arbitrary to the prior version (1.1.3) until we can bump all CI workers to 1.63
Thanks for working through this @rrichardson! This is an amazing effort 🙇