-
Notifications
You must be signed in to change notification settings - Fork 37
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
introduce generate_overflowing and increment_overflowing methods #75
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,35 @@ impl Generator { | |
self.generate_from_datetime(crate::time_utils::now()) | ||
} | ||
|
||
/// Generate a new Ulid. Each call is guaranteed to provide a Ulid with a larger value than the | ||
/// last call. The time part of the returned [`Ulid`] can be up to one millisecond in the future, | ||
/// under reasonable assumptions about processor speeds. | ||
/// | ||
/// # Panics | ||
/// | ||
/// This function panics if the previously returned value was already the highest possible ulid. | ||
/// This should not happen before year 91000 of the gregorian calendar at least. | ||
/// | ||
/// ```rust | ||
/// use ulid::Generator; | ||
/// let mut gen = Generator::new(); | ||
/// | ||
/// let ulid1 = gen.generate_overflowing(); | ||
/// let ulid2 = gen.generate_overflowing(); | ||
/// | ||
/// assert!(ulid1 < ulid2); | ||
/// ``` | ||
pub fn generate_overflowing(&mut self) -> Ulid { | ||
let next = Ulid::new(); | ||
if next > self.previous { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This behavior differs from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to handle this one once #75 (comment) is decided, as it'll impact the things to do :) |
||
self.previous = next; | ||
next | ||
} else { | ||
self.previous = self.previous.increment_overflowing(); | ||
self.previous | ||
} | ||
} | ||
|
||
/// Generate a new Ulid matching the given DateTime. | ||
/// Each call is guaranteed to provide a Ulid with a larger value than the last call. | ||
/// If the random bits would overflow, this method will return an error. | ||
|
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.
If a single version of
generate_overflowing
is going to be added I'd prefergenerate_from_datetime_with_source_overflowing
. This would give the user the most choice.However, looking at this code again, it may make sense to make the overflow value part ofMonotonicError
. If you're up for that change it should provide the best overall API here.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.
Nevermind this doesn't work since it wouldn't store into
previous
.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.
Considering the reason to be of
generate_overflowing
is for people who just want to say "I don't care", I think only havinggenerate_from_datetime_with_source_overflowing
would be counter-productive: after all, it's reasonably easy to replace it:So IMO this is the place where there's literally the least to gain, compared to
generate_overflowing
that does gain a lot compared to trying to reimplement it manually. Now, if you'd rather have all the variants, I'm all good with adding them; I was just trying to avoid combinatorial explosion of the API :)I'll try to handle your other review comments soon! In the meantime please let me know your thoughts about the API you'd prefer so I can adjust this PR accordingly :)
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.
@dylanhart WDYT about this question?