Actually I would much prefer to follow: https://github.com/discourse/discourse/blob/master/app/models/user_search.rb care to give that a shot?
also do we have a cla in place … jeez we really need a cla bot.
Yep, CLA signed before I made this PR
@SamSaffron Regarding casing in SQL – is there a reason why you’d prefer to go all lower-case?
What’s nice about the distinction between upper and lower is that it’s more clear to see what was templated-in when examining logs. (Also: bonus aesthetic value of matching the SQL logging style of Rails itself.)
Either way this bikeshed gets painted / cargo cult gets called, I’m game to help make this codebase more stylistically consistent
I personally prefer mixed case as well, where SQL keywords are all upper case and our keys are lower case, to match the Rails logs. I’ve already done this in the code base so we have a mixed style, see:
I don’t want to bike shed too much, but we should decide on one format and stick to it
@eviltrout Right – that’s the style in which I made this little change.
How do you guys feel about adopting GitHub’s Ruby style guide? https://github.com/styleguide/ruby
@edward I prefer lower case on a personal level, but for Discourse agree, we should go upper case.
As to style guide, can we discuss this on http://meta.discourse.org under the dev category ?
anyway, regardless, moving search to sql_builder is a huge win over string concats. will simplify the code muchly
@SamSaffron Ok, cool. So… is this PR good to merge then?
sure its fine, let me show you what I had in mind … one sec
arghh … I see its actually a complex refactor …
@SamSaffron I wish AREL was easier to work with too It sucks to end up writing giant amounts of SQL that later turn out to be less flexible than hoped.
Thanks for merging my patch