Skip to content
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

added helper method for COMB UUIDs #180

Closed
wants to merge 1 commit into from
Closed

added helper method for COMB UUIDs #180

wants to merge 1 commit into from

Conversation

1ma
Copy link
Contributor

@1ma 1ma commented Sep 3, 2017

This PR add a helper method to the Uuid class for generating sequential COMB UUIDs, as discussed on issue #53.

Disclaimer: this proposal is a WIP. Though the feature seems to work as intended I've decided to open the PR early on to get feedback on the approach (feels a bit clunky to me). I'd also like input on how to approach testing such feature.

@ramsey
Copy link
Owner

ramsey commented Sep 4, 2017

At first glance, the approach looks good to me. I can take a more detailed look later this week and offer some pointers on testing. Thanks!

{
$currentCodec = $this->getCodec();
$combGenerator = new CombGenerator($this->getRandomGenerator(), $this->getNumberConverter());
$combCodec = new TimestampFirstCombCodec($this->getUuidBuilder());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a difference between a COMB UUID and a Timestamp First COMB UUID. I'm worried that that name of the method comb() causes confusion, since you're using the TimestampFirstCombCodec() internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed but TimestampFirstCombCodec is the one that produces monotonically increasing UUIDs, which is the whole point.

Do you have an alternative name in mind?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe timestampFirstComb()?

@ramsey
Copy link
Owner

ramsey commented Jan 13, 2018

Any other thoughts on choosing a different name? I still disagree with using comb(), since it's not a standard COMB UUID but is a timestamp-first COMB UUID.

@1ma
Copy link
Contributor Author

1ma commented Jan 17, 2018

It's ok with me. It appears that I managed to nuke my fork though, sorry. I'll resubmit the PR (with the name of the helper set to timestampFirstComb())

@1ma 1ma closed this Jan 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants