-
Notifications
You must be signed in to change notification settings - Fork 123
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
Conversation
@jashmenn We would love to get your feedback on this PR. |
Hmm yeah, we added it that way because it matched the MySQL 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 ? |
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. |
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. |
Yes, finders work just as we expect. |
Okay great, I'll merge this - we'll have to bump the major version for the next release. Thanks!! |
Observe ActiveRecord conventions
Thanks! Dan Kohn mailto:[email protected] On Fri, Aug 14, 2015 at 7:16 PM, Nate Murray [email protected]
|
Patches for #59 and #66.
Note snippet in #66 to override the default key.