search

jashmenn / activeuuid

Binary uuid keys in Rails
MIT License
340 stars 124 forks source link

Observe ActiveRecord conventions #67

Closed ivanoblomov closed 9 years ago

ivanoblomov commented 9 years ago

Patches for #59 and #66.

Note snippet in #66 to override the default key.

dankohn commented 9 years ago

@jashmenn We would love to get your feedback on this PR.

jashmenn commented 9 years ago

Hmm yeah, we added it that way because it matched the MySQL HEX function (https://github.com/jashmenn/activeuuid/commit/4e418f4c6ba321ef85a10fb52594aae3b22567a6) but I can see why you'd want it to return the string UUID.

It's tricky, this is a breaking change and we might need a major version increment. Can you speak more to why we want to do it this way? I haven't worked closely with this code in a while and I want to be careful about breaking existing users' code. Does anyone else want to weigh in? /cc @pyromaniac ?

dankohn commented 9 years ago

The context is that we had our whole app working with UUID ids with Postgres, and then needed to switch to MySQL because of changing requirements. The built-in Rails support for UUID ids in Postgres returns UUIDs (not the MySQL HEX output), and so it seems that activeuuid should as well.

Among other things, I would hope that this gem might eventually get accepted into Rails core.

But, I certainly agree that you would want to rev the version number.

jashmenn commented 9 years ago

Ah - got it. Also, my main concern is ensuring that we're able to use uuids as conveniently as regular ids. Can you confirm for me that this pull req doesn't break the finders? It seems like it should work, but I haven't had a chance to try it out.

dankohn commented 9 years ago

Yes, finders work just as we expect.

jashmenn commented 9 years ago

Okay great, I'll merge this - we'll have to bump the major version for the next release. Thanks!!

dankohn commented 9 years ago

Thanks!

Dan Kohn mailto:dan@dankohn.com tel:+1-415-233-1000

On Fri, Aug 14, 2015 at 7:16 PM, Nate Murray notifications@github.com wrote:

Okay great, I'll merge this - we'll have to bump the major version for the next release. Thanks!!

— Reply to this email directly or view it on GitHub https://github.com/jashmenn/activeuuid/pull/67#issuecomment-131265730.