introduce generate_overflowing and increment_overflowing methods #75
Conversation
| /// ``` | ||
| pub fn generate_overflowing(&mut self) -> Ulid { | ||
| let next = Ulid::new(); | ||
| if next > self.previous { |
There was a problem hiding this comment.
This behavior differs from Generator::generate(). In the same-millisecond case generate will always increment while this will first try a random value. I'd like to preserve the existing behavior because it matches the reference implementation.
There was a problem hiding this comment.
I'm going to handle this one once #75 (comment) is decided, as it'll impact the things to do :)
| /// | ||
| /// assert!(ulid1 < ulid2); | ||
| /// ``` | ||
| pub fn generate_overflowing(&mut self) -> Ulid { |
There was a problem hiding this comment.
If a single version of generate_overflowing is going to be added I'd prefer generate_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 of MonotonicError. If you're up for that change it should provide the best overall API here.
There was a problem hiding this comment.
Nevermind this doesn't work since it wouldn't store into previous.
There was a problem hiding this comment.
Considering the reason to be of generate_overflowing is for people who just want to say "I don't care", I think only having generate_from_datetime_with_source_overflowing would be counter-productive: after all, it's reasonably easy to replace it:
gen.generate_from_datetime_with_source_overflowing(time, source);
// is equivalent to
gen.generate_from_datetime_with_source(time, source)
.unwrap_or_else(|| gen.generate_from_datetime_with_source(time + Duration::from_millis(1), source).unwrap());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.
@dylanhart WDYT about this question?
Fixes #71
I did not add all the
_with_datetime_and_sourcevariants to avoid combinatorial explosion: people who want the_overflowingvariant are most likely the ones who do not care about the details of how the ulid is generated anyway.