Infix improvements #134

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants

These two commits help you do some additional math bits with SUM, COUNT, etc that you could not easily do otherwise.

stormsilver added some commits Aug 22, 2012

Added additional parentheses around infix operations.
In situations like (10/(5*2)), the infix to_sql did not generate enough parens
This adds parentheses around every infix operation. Yes, it will double up
on parens in some cases, but that should not hurt anything.
Teach Arel::Nodes::Function to do Arel::Math
Sometimes you'd like to be able to do table[:weight]/10. Simply including the
Math module allows the Sum node to accept the math operators.

This pull request passes (merged 077cc47 into e032dab).

Collaborator

ernie commented Aug 22, 2012

I'm not sure it's ARel's job to add those parentheses around infix operations automatically. It seems to me that if that's wanted, it'd be expressed by wrapping them in a Grouping node.

This pull request passes (merged f539387 into e032dab).

Oh, cool, so GitHub automatically updates this PR with additional commits, even when I didn't ask it to. :( Please ignore f539387... that should NOT be part of this PR.

Re: not ARel's job. Of course you are the expert - what do I know? However, I would disagree. If ARel lets you do something like column1/(column2*column3), then it should take care to ensure that the expression works properly. If you still disagree, then feel free to close the PR.

Collaborator

ernie commented Aug 22, 2012

This reminds me of a similar discussion I had with @tenderlove at least a year ago. :)

Anyway, as I recall, our stance on ARel has been that it will create the AST that you ask it to, but it doesn't claim responsibility for creation of an AST that infers what you really want -- that is, if you want a grouping, you should ask for a grouping.

I do sympathize with what you're saying, in this case, so not closing the PR just yet, in case @tenderlove wants to weigh in.

nertzy commented May 19, 2013

9 months later with no response, probably time to close this out? For what it's worth, I agree with @ernie that you can just use a Grouping when you want the parentheses. As someone who has only just jumped into deeper ARel usage, I admit it was a bit difficult to figure out I needed a Grouping, so perhaps there's something that could be done to shorten the learning curve.

Collaborator

ernie commented May 19, 2013

Yup, sounds like a plan to me.

@ernie ernie closed this May 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment