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

Use Arel for SQL construction #372

Closed
wants to merge 1 commit into from
Closed

Conversation

dmitry
Copy link

@dmitry dmitry commented May 24, 2014

No description provided.

@dmitry
Copy link
Author

dmitry commented May 25, 2014

@batter

Build is failed because of the database_cleaner and the ruby 1.8 version. It's not supported anymore: DatabaseCleaner/database_cleaner@8e448be (database_cleaner 1.3.0 released yesterday)

Everything else is in a good working condition.

@batter
Copy link
Collaborator

batter commented May 27, 2014

Thanks for the pull request. Is there an advantage to using Arel for sql construction, or is it just cleaner syntax? I like it, just curious about what the pros & cons are if there are any.

@dmitry
Copy link
Author

dmitry commented May 27, 2014

Basically there are two things Arel is used for (from the Arel readme):

  • Simplifies the generation of complex SQL queries
  • Adapts to various RDBMSes

In my case I want paper_trail to be more cleaner and database agnostic.

I only know one drawback: performance is a little lower, but not really much.

@batter batter changed the title Use ARel for SQL construction Use Arel for SQL construction May 28, 2014
@batter batter closed this in 4601926 May 28, 2014
@batter
Copy link
Collaborator

batter commented May 28, 2014

@dmitry - Thanks again for the PR. I took most of your changes and implemented them when possible but condensed some syntax and took out some of the unnecessary changes in 4601926. I hope you approve.

I like to leave calls from one method to another method that is defined within the same module predicated with self. to indicate that the method is defined on the same module. I also like to check not just for truthiness, but also that an argument actually is true or false when checking to see if an argument is receiving one of those (to ensure the user actually intends for that boolean factor to be triggered) and avoid any possible confusion when a method gets invoked. Hope that makes sense. 😃

@dmitry
Copy link
Author

dmitry commented May 29, 2014

@batter ok, didn't knew about coding style. Hope some day to be one of the contributers to a paper_trail ;)

@batter
Copy link
Collaborator

batter commented May 29, 2014

Well I'll add you to the README list of contributors since you are a contributor. I wanted to grab your actual commit but since I rejected a good portion of the changes you had made to that commit, I didn't want to merge it and revert since that would mess up the git history for the file... Not sure if I could've just grabbed the commit and manually undid those and then re-committed it... but at any rate, you're a contributor in my book! Thanks again.

@batter batter added this to the 3.0.3 milestone Jun 3, 2014
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.

2 participants