rails / arel

A Relational Algebra
2.06k stars 390 forks source link

Introduce Expression Node #449

Closed kbrock closed 6 years ago

kbrock commented 7 years ago

This is a followup to #435

This strives to alleviate the inconsistencies in the arel interface:

select(arel_table[:name]) works but select(arel_table[:name].lower) does not.

Or arel_table[:name].asc works but arel_table[:name].lower.asc does not.

Many nodes can be used in places that are not currently characterized by the current Arel::Node. This introduces these common nodes in a way that makes it easier for rails to use common shortcut methods like desc, lower or other common methods.

SELECT "users"."id", "users"."name"
FROM "users"
ORDER BY LOWER(CASE "users"."address" IS NULL then "A" else "users"."address") DESC

(yes, this is not the sql I would have written, but I was trying to generate sql that I could imagine generating through rails)

Thanks again for being so helpful extending rails and Arel to the many needs of your users.

rails-bot commented 7 years ago

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

kbrock commented 7 years ago

Rebased to make it recent (but didn't introduce any changes)

@matthewd any comments to make this easier to merge?

kbrock commented 6 years ago

rebased to make it recent (but didn't introduce any changes)

@sgrif or @matthewd Any comments to make this easier to merge?

thnks

matthewd commented 6 years ago

@kbrock test failures sound relevant. Otherwise I think this is good to go.

Sorry I've been terrible about merging PRs for a while, and even worse about going back over those that I had neglected 😞

kbrock commented 6 years ago

@matthewd very sorry to send you a false signal.

Thanks for all the help and hope all is well.

kbrock commented 6 years ago

Does this need a :nodoc:?

 module Arel
   module Nodes
-    class NodeExpression < Arel::Nodes::Node
+    class NodeExpression < Arel::Nodes::Node # :nodoc:
       include Arel::Expressions
matthewd commented 6 years ago

Does this need a :nodoc:

Arel isn't really nodoc-heavy, so I think this is fine. :+1: