Skip to content

sql comments support #157

Open
wants to merge 1 commit into from

4 participants

@grosser
grosser commented Jan 8, 2013

@tenderlove as discussed here

scope = User.where(aaa).comment("XXX")
scope.comment("YYY")
scope.to_sql == "SELECT * FROM users WHERE xxx /* XXX YYY */"
@tamird
tamird commented Sep 16, 2014

@grosser are you planning on picking this up?

@grosser
grosser commented Sep 16, 2014

if someone things it's mergeable I'll gladly rebase + fix the tests

@tamird
tamird commented Sep 16, 2014

seems mergeable to me. it would enable a gem that instruments AR to place comments on individual clauses a la https://github.com/basecamp/marginalia

@grosser
grosser commented Sep 16, 2014

@matthewd @rafaelfranca would you be willing to merge this once rebased / passing tests ?

@rafaelfranca
Ruby on Rails member

👍

@grosser
grosser commented Sep 17, 2014

it is done!

@grosser
grosser commented Sep 17, 2014

FYI tests fail on master for me ...

  1) Error:
the to_sql visitor#test_0020_should visit_Set:
ArgumentError: wrong number of arguments (1 for 2)
    /Users/mgrosser/Code/tools/arel/lib/arel/visitors/reduce.rb:6:in `accept'
    /Users/mgrosser/Code/tools/arel/test/visitors/test_to_sql.rb:233:in `block (2 levels) in <module:Visitors>'
@tamird tamird commented on an outdated diff Sep 17, 2014
lib/arel/select_manager.rb
@@ -234,6 +234,12 @@ def source
@ctx.source
end
+ def comment comment
+ @ast.comment ||= []
@tamird
tamird added a note Sep 17, 2014

s/comment/comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tamird tamird commented on an outdated diff Sep 17, 2014
lib/arel/tree_manager.rb
@@ -27,7 +27,9 @@ def visitor
def to_sql
collector = Arel::Collectors::SQLString.new
collector = visitor.accept @ast, collector
- collector.value
+ sql = collector.value
+ sql << " /* #{@ast.comment.join(" ")} */" if ast.comment
@tamird
tamird added a note Sep 17, 2014

nit: single quotes on the join argument

@tamird
tamird added a note Sep 17, 2014

also pluralize to comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tamird tamird and 1 other commented on an outdated diff Sep 17, 2014
lib/arel/nodes/node.rb
@@ -5,6 +5,8 @@ module Nodes
###
# Abstract base class for all AST nodes
class Node
+ attr_accessor :comment
@tamird
tamird added a note Sep 17, 2014

maybe this should be implemented as a separate comment node?

@grosser
grosser added a note Sep 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tamird
tamird commented Sep 17, 2014

new node would be implemented similarly to the AliasPredication module

@grosser
grosser commented Sep 17, 2014

@rafaelfranca ok as is or refactor to node ?

@rafaelfranca
Ruby on Rails member

I think it is fine as it is.

@grosser
grosser commented Sep 17, 2014

then please hit that merge button :D

@seuros
Ruby on Rails member
seuros commented Sep 17, 2014

Le Travis need to be 💚

@rafaelfranca
Ruby on Rails member

Yes. I'll rerun the test.

@rafaelfranca
Ruby on Rails member

Ah, please change the commit message to better describe the addition.

@grosser
@grosser
@tamird
tamird commented Sep 17, 2014

oh, i misread this initially. since this is on the select manager, i don't think it's as useful as it could be. could we put the comments on nodes?

ideally, this would allow arbitrary commenting in sql statements, not just selects

@grosser
grosser commented Sep 18, 2014

yeah ... I was exited when User.where(:id => 1).comment('xxx').to_sql worked ... but turns out User.where(:id => 1).comment('xxx').first does not work, so not sure if I'm on the right path and AR needs a patch or if I'm on the wrong path ... any idea @rafaelfranca

@grosser
@tamird
tamird commented Sep 18, 2014

@grosser yeah, comments on nodes won't get you there - that's going to have to be implemented on AR relations. however, comments on nodes will let you hook into the AR internals and add instrumentation to generate comments like WHERE created_at > '2014-09-17' /* app/controllers/foo_controller.rb:57 */ which can be really handy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.