Skip to content

Use Arel for SQL construction#372

Closed
dmitry wants to merge 1 commit intopaper-trail-gem:masterfrom
dmitry:arel
Closed

Use Arel for SQL construction#372
dmitry wants to merge 1 commit intopaper-trail-gem:masterfrom
dmitry:arel

Conversation

@dmitry
Copy link
Copy Markdown

@dmitry dmitry commented May 24, 2014

No description provided.

@dmitry
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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