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

Observe ActiveRecord conventions #67

Merged
merged 3 commits into from Aug 14, 2015
Merged

Observe ActiveRecord conventions #67

merged 3 commits into from Aug 14, 2015

Conversation

ivanoblomov
Copy link
Contributor

Patches for #59 and #66.

Note snippet in #66 to override the default key.

@dankohn
Copy link

dankohn commented Aug 14, 2015

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

@jashmenn
Copy link
Owner

Hmm yeah, we added it that way because it matched the MySQL HEX function (4e418f4) 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
Copy link

dankohn commented Aug 14, 2015

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
Copy link
Owner

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
Copy link

dankohn commented Aug 14, 2015

Yes, finders work just as we expect.

@jashmenn
Copy link
Owner

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

jashmenn added a commit that referenced this pull request Aug 14, 2015
@jashmenn jashmenn merged commit 3b0a26f into jashmenn:master Aug 14, 2015
@dankohn
Copy link

dankohn commented Aug 14, 2015

Thanks!

Dan Kohn mailto:[email protected]
tel:+1-415-233-1000

On Fri, Aug 14, 2015 at 7:16 PM, Nate Murray [email protected]
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
#67 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants