-
Notifications
You must be signed in to change notification settings - Fork 190
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
Uuid 2.0 RFC #615
base: main
Are you sure you want to change the base?
Uuid 2.0 RFC #615
Conversation
Thanks for writing this @rrichardson! I'll spend some proper time reading through the RFC, but wanted to set the expectation early that since we committed to a stable |
docs/rfcs/uuidgen-refactor.md
Outdated
trait SequenceSource<T: TimeSource> { | ||
type Output; | ||
fn next(&mut self, tm: <T as TimeSource>::Output) -> Self::Output; |
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.
Rather than making this part of SequenceSource
, what about requiring that TimeSource::Output
implements Eq
, and then deciding inside the UUID library whether to check for duplicates or not?
I'm also wondering to what extent it makes sense for TimeSource::Output
to be parameterized, without having some notion of precision with which to turn it into the component needed for a UUID? Or is the expectation that the library will nonetheless rely on specific output types rather than being generic?
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.
I definitely like the idea of removing any awareness of TimeSource
from SequenceSource
. However, whether or not the SequenceSource
checks the time for duplicates isn't really a version-specific thing, it's a preference of the implementation. If we could pass in some notion of "there was a conflict" into SequenceSource::next()
that would be fine by me. Maybe an Option<bool>
Regarding Output precision: My assumption was that the precision required would be version-specific, so the Output
would always be parameterized by each UuidV#
Implementation. The only exception to this, though, would be UuidV8, which violates this rule.
TBH, this GenBuilder API was inspired by the idea that one could use a builder to make a highly tuned UuidV8 implementation by choosing how many bits they wanted for seconds, frac-seconds, random and sequence. However, there might be better/safer ways to accommodate that level of flexibility in the API design. (I was going to add the precision/size check at runtime in the Builder construction)
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.
@rrichardson That does seem like perhaps more flexibility than necessary.
And yeah, passing in "there was a conflict" seems like an improvement, at least.
👍 for the proposal to have feature flags only for dependencies. As someone with crates that indirectly depend on uuid, I would appreciate being able to prune dependencies. (Please consider having all of those feature flags not part of |
Much of this proposal could be tacked on without changing the existing API. Firstly, one can't create a Timestamp without creating an Secondly, the Traits and timestamps assume too much. They are fixed at u64, u32, and u16. This, again, makes certain version implementations a bit awkward and they don't compose well. We should be able to use a SequenceGenerator to generate 128 bit random numbers to serve the v7 and v8 use cases. Thirdly, things like Fourthly, (and less important) the Lastly, I think we need to refactor how features are done, because, IMO, it is a bit of a mess. If we do this, we do need to make sure it's done in a way that is backwards compatible, or at least define an evolutionary path. |
…nowledge of SequenceSource
Since this is a large API change, even for a v2.0. What are your thoughts on putting the new implementation under a new module (called |
I support this approach. Allows experimentation without affecting current users. |
Thanks for writing this up @rrichardson. At first, this seems like a lot of machinery to generate UUIDs, and I'm not sure that splitting UUIDs into separate types by version is necessarily the best user experience. It seems like the main problem we've got is that the responsibilities of our I'd welcome any experimentation to build a better API as an external library, but I think the bar for heavy refactoring at this point is high. The end-user pay-off really needs to justify shifting things around, especially when the basic APIs have remained the same for many years. |
One major version I would be keen to do now was one that simply re-exported the entire current API, but made the |
RFC proposal
Rendered: https://github.com/rrichardson/uuid/blob/rfc-app-2/docs/rfcs/uuidgen-refactor.md
Description
This is the core of my proposal for a refactor of the Uuid crate. Fundamentally, it doesn't change how the existing APIs are used a whole lot, it introduces a TimeSource and changes the ClockSequence to SequenceSource (where a sequence can either be monotonic or random).
The biggest changes are:
Related Issue(s)
#612