Skip to content

Commit

Permalink
Baby::Squeel::Nodes::Node should be sure to include Arel::Math
Browse files Browse the repository at this point in the history
  • Loading branch information
Ray Zane committed Sep 14, 2017
1 parent c399008 commit cf7d7d0
Showing 1 changed file with 5 additions and 0 deletions.
5 changes: 5 additions & 0 deletions lib/baby_squeel/nodes/node.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ module Nodes
# In the future, these proxy classes should be more specific and only
# include necessary/applicable modules.
class Node < Proxy
def initialize(node)
super
node.extend Arel::Math
end

extend Operators::ArelAliasing
include Operators::Comparison
include Operators::Equality
Expand Down

3 comments on commit cf7d7d0

@chewi
Copy link
Contributor

@chewi chewi commented on cf7d7d0 Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is a real issue or not but this changes the behaviour of + on SQL literals. This is a very convoluted example but I used to do stuff like sql('CASE WHEN ') + COALESCE(foo, bar).to_sql + sql(' ELSE baz END') and this started complaining with unsupported: String. I've now realised that if you're finishing with a bunch of concatenated strings and SQL literals then you can just stick to strings entirely like 'CASE WHEN ' + COALESCE(foo, bar).to_sql + ' ELSE baz END'. That's not a problem but I'm wondering whether there are other legitimate uses for +. The method is actually derived from the String class and there are no aliases that you can use instead.

@rzane
Copy link
Owner

@rzane rzane commented on cf7d7d0 Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's an interesting point. If you'd like to send a PR, we could have a dedicated node for SQL literal, and change the case expression here:

baby_squeel/lib/baby_squeel/nodes.rb

Line 20 in b97fa0e

when Arel::Nodes::Node, Arel::Nodes::SqlLiteral
.

when Arel::Nodes::SqlLiteral
  SqlLiteral.new(arel)
when Arel::Nodes::Node
  Node.new(arel)

@chewi
Copy link
Contributor

@chewi chewi commented on cf7d7d0 Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I actually confused two issues slightly. The unsupported: String error went away after wrapping the to_sql stuff in sql(). The + issue resulted in + actually appearing in the final SQL string.

As I said, I realised that my code was overly complicated anyway but a dedicated node could still be useful so I'll try to come up with a PR when I have a moment. Things are a bit heavy this week.

Please sign in to comment.