Remove "final" keyword to allow extending

[dvankley]

No description provided.

[ramsey]

Can you provide more details/use cases on why you wish to extend the Uuid class? The choice of using the final keyword was purposeful.

[dvankley]

Sure thing. We're looking to use a modified version of the type 1 UUID to allow us to generate unique type 1 UUIDs for past data. We're customizing the UUID because the data was not stored with 100 nanosecond resolution timestamps and we still need to guarantee uniqueness. We can't use other UUID types because we're using UUIDs as the primary ids for time series data in Cassandra (https://github.com/Pardot/Rhombus), so it's essential to have the ids sortable by time for range queries to work correctly. Initially we just created a class to wrap and manipulate your standard UUIDs to do what we want, but some functions (such as getTimestamp) validate the variant field, which we are using to denote that our timestamps are non-standard. So we'd like to extend the UUID class to make our code cleaner and properly recognize our modified variant field.

[stanlemon]

Hey @ramsey just curious if you could expand on why it was purposeful?

[ramsey]

@marijn, would you like to weigh-in on this? I know you were advocating for the use of the final keyword a while back.

[marijn]

Hell yes. On the road right now but will reply tomorrow.

Verstuurd vanaf mijn iPhone

Op 7 apr. 2014 om 17:41 heeft Ben Ramsey notifications@github.com het volgende geschreven:

@marijn, would you like to weigh-in on this? I know you were advocating for the use of the final keyword a while back.

—
Reply to this email directly or view it on GitHub.

[eduardosoliv]

+1 remove final

[marijn]

@dvankley would you mind posting some of that old UUID wrapper class that you have? Perhaps also a test if you have one? I don't really understand what you're doing what can't be done without inheritance...

[ramsey]

One of the primary reasons for the use of "final" is that this is based on a specification. Therefore, it can be trusted (I hope) to match the spec. A subclass might not follow the spec, so anyone using that subclass cannot trust that the UUIDs they are generating follow the spec. Additionally, the UUID should be immutable. Keeping the class "final" ensures that subclasses do not try to introduce mutability.

@dvankley, one idea I have that might help is for Rhumsaa\Uuid to provide a fromTime() method (like its fromString() method). Currently, to test proper UUID creation from known timestamps, I use a static property that I set to a value that looks like the output of PHP's gettimeofday() function. Then, when creating version 1 UUID's I can ensure that they are identical to what I expect. See here for an example:

uuid/tests/UuidTest.php

Line 954 in c43be02

Uuid::$timeOfDayTest = $timeOfDay;

So, a fromTime() method would work similarly. You could pass in specific time properties (like those returned from gettimeofday()), along with an optional node and clock sequence, and this would generate a UUID according to previously-determined times. This might work for your needs, rather than having to create your own UUID variant.

Thoughts?

[dvankley]

Your reasoning behind using "final" makes sense, but I'm not quite on board with the conclusion. To me it seems like even without "final" your class is still guaranteed to follow the spec and be immutable. I would say responsibility for making those and any other guarantees for any subclasses would fall on their writers, and users of those subclasses would have to consider the contract of the subclass they were using.

The fromTime() method would be helpful, but we're already using the fromString() method with a little bit of sprintf to achieve basically the same thing, so it's probably not worth your time to implement just for us. Thanks for the offer, though.

[marijn]

@dvankley I'm still interested in seeing the original test and corresponding class. Perhaps that will make it possible to provide you better arguments why final is the way to go.

[dvankley]

The module we're using it in isn't open source, so my company would probably get pissy if I posted it here. Really the only stuff we're doing beyond just the standard building UUIDs from component fields is storing some additional information in the timestamp value to guarantee uniqueness, since all our incoming timestamps are only second precision. It's nothing fancy and works fine in a wrapper class, but I thought it would be cleaner from a user's perspective to have it as a subclass.

[marijn]

but I thought it would be cleaner from a user's perspective to have it as a subclass.

Composition > Inheritance. Wrapping is way better in my opinion.

[stanlemon]

You can make that argument, but I think all you're doing is encouraging someone to fork your repository unnecessarily. I'll agree with you that I prefer composition over inheritance, but I prefer choice even more so. I think if your UUID class was a value object you'd have a better case for making it immutable with final. However, your value and construction are interwoven and ergo my disagreement with the use of final here.

[ramsey]

After much thought and discussion with others, I am inclined to accept this pull request to remove the final keyword.

Any other thoughts or rebuttals?

[ramsey]

Also, when I accept this PR, it will resolve #23.

[dvankley]

Sounds good to me!

[marijn]

I don’t think it’s necessary — albeit that I haven’t had the time to formulate alternatives due to a lack of time — especially not for the reason of extensibility.

[ramsey]

I don't think it's necessary, either, but I'm failing to see good arguments for keeping the class final. People will naturally want to try to extend it and add to it, and I don't see a reason anymore why we shouldn't allow that.

[marijn]

Because you'll have to support the public and protected API from that point onwards.

[stanlemon]

@marijn that's already being done... they're called tests.

[marijn]

@marijn that's already being done... they're called tests.

What are you trying to say with that? I'm not sure I understand you.

My point is that if Ben decides to drop the final keyword all protected methods and fields have to be kept backwards and forwards compatible if he wants to keep releasing semantic versions.
If he doesn't all he has to do is support the public API.
I'm not sure how your comments regarding tests relate to my support argument.

[stanlemon]

Semver allows changing of protected/private code in a Y release. It restricts changes to the public API through X. Removing final doesn't suddenly make things not exposed by the class contract locked in on semver. Check out item 7 on semver.

[marijn]

My mistake, I thought this would considered part of the public API.

[ramsey]

I merged this into the 3.0 branch in commit dc1b735.

Thanks!