iconara / cql-rb

Cassandra CQL 3 binary protocol driver for Ruby
106 stars 31 forks source link

Make Cql::Uuid behave more like SimpleUUID::UUID #82

Closed outoftime closed 10 years ago

outoftime commented 10 years ago

One stumbling block when making the switch from cassandra-cql to cql-rb is the different UUID type returned from queries. This pull just adds a couple of method aliases to Cql::Uuid that makes it work with common use cases that were written around SimpleUUID:

iconara commented 10 years ago

Sorry for taking so long to comment.

I like the idea of adding #to_i as an alias for #value, but I don't think that adding #to_guid is a good idea. The string representation of a UUID is not commonly known as a GUID, so the method is badly named in SimpleUUID, in my opinion. Also, SimpleUUID is just one out of many other UUID libraries for Ruby, and I don't see any reason to add a method just to be compatible with one of them. If you really need it I think your best course of action is to monkey patch it in your code.

outoftime commented 10 years ago

@iconara thanks for the response—not to worry about the delay. I'm down to modify the PR to take out to_guid, but I do want to see if I can convince you first : )

SimpleUUID is just one out of many other UUID libraries for Ruby, and I don't see any reason to add a method just to be compatible with one of them

That's true, but I think it's safe to assume a lot of people have switched/will switch from cassandra-cql to cql-rb. Since cassandra-cql uses UUIDs backed by SimpleUUID, it's uniquely useful to make the APIs between those two representations API compatible.

The string representation of a UUID is not commonly known as a GUID, so the method is badly named in SimpleUUID, in my opinion

Agreed, but I'm not sure I see much harm in exposing to_guid. The main downside would be if to_guid could reasonably be interpreted as something else, leading to confusion or conflicts with another potential meaning/implementation of to_guid down the line. I don't see much danger of that in this case.

So my claim would be that aliasing to_guid would have a considerable upside (making things easier/less astonishing for apps porting from cassandra-cql to cql-rb) and not much downside. That said, if I've failed to convince you, let me know and I'll update the pull request to only expose to_i. Thanks!

iconara commented 10 years ago

The harm is adding method aliases that need to be supported forever. I think #to_i is a good one and worth supporting, but not #to_guid. cql-rb is not a drop-in replacement for cassandra-cql, and there are so many other things you need to change that I don't think adding #to_guid has any real benefit.

It might seem like a tiny thing to quibble about, but I take the API very seriously. That's one of the reasons I decided to implement a UUID library in cql-rb, even when there were alternatives. I didn't like their implementations and I didn't like their APIs.

Thanks a lot for the suggestion, I value all input even when I don't agree with it. I think that merging a PR is more work than just adding the alias myself, so you don't have to do anything. I'll be sure to mention you in the release note, and I'll leave this PR open until the alias is in master.

outoftime commented 10 years ago

@iconara Thanks for hearing me out : )

cql-rb is not a drop-in replacement for cassandra-cql, and there are so many other things you need to change that I don't think adding #to_guid has any real benefit

With that in mind, I wonder what you'd think of Cequel including the to_guid monkey patch? The 1.1 release of Cequel will be the first to use cql-rb, and I'm concerned that because of the change in UUID implementation, it's technically a breaking change (otherwise the change is transparent to Cequel users API-wise). So anything I can do to reduce the breakage for Cequel users would make me feel better about that : )

Nothing actionable on your end there, just would love to hear your thoughts.

iconara commented 10 years ago

I think that you should add the monkey patch in 1.1 of Cequel, mark it as deprecated and then remove it in 2.0.

outoftime commented 10 years ago

@iconara cool – was thinking along those lines myself. thanks!

nirvdrum commented 10 years ago

I never liked how SimpleUUID used to_guid to mean to_s. But, I had to accept it and the codebase is now littered with those calls. We're in the midst upgrading from Cequel 0.5.6 to Cequel master (1.1) and going from cassandra-cql to cql-rb, along with Cassandro 1.1.9 to 2.0.5. Without a doubt, there was a fair bit to change in switching to CQL 3.0, but the UUID differences were the most insidious. Mostly because they're effectively meaningless and permeated throughout.

Wherever it ends up, I agree the call should be added as a deprecated shim. Being able to print out a warning (that can also be turned off) would let people get on with their lives and pick away at converting to the more natural to_s.