Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return and instance of Arel::Attributes when calling [] on the model. #264

Closed
wants to merge 1 commit into from

Conversation

sikachu
Copy link
Member

@sikachu sikachu commented Apr 12, 2011

Hi,

I've discussed with Aaron a while ago about the way that I can use ARel's comparison methods (#gt, #lt, and so on) in AR. Turned out that I have to access it via #arel_table method of the model:

users = User.arel_table
famous_users = User.where(users[:fame].gt(80))

This become a bit tedious as I have to have another variable to store an instance of Arel::Table to be able to call those methods on it. Also, I don't think the code look clean if I merge those two lines together and make it as

famous_users = User.where(User.arel_table[:fame].gt(80))

So, after some discussion, I think it's better to make AR .[] method to return the instance of Arel::Attributes::*. This will give more readability to the code example above, and I think it fits in great with the context.

After applying my patch, you would then can do

famous_users = User.where(User[:fame].gt(80))

I think that looks really nice. I really wish I could get some feedback from you guys.

So, now you can do something like this:

  Topic.where(Topic[:ratings].gt(5))

Without having to call `#arel_table` first.
@svenfuchs
Copy link

@pusewicz
Copy link

+1

@jeroenvandijk
Copy link
Contributor

I've used it too +1 re https://twitter.com/#!/tenderlove/status/57839406987493376

@burke
Copy link
Contributor

burke commented Apr 12, 2011

+so many. I use arel_table constantly. It feels clumsy having to type it everywhere. (re https://twitter.com/#!/tenderlove/status/57839406987493376)

@john-griffin
Copy link

+1

3 similar comments
@tadast
Copy link
Contributor

tadast commented Apr 12, 2011

+1

@dombesz
Copy link

dombesz commented Apr 12, 2011

+1

@rougecardinal
Copy link

+1

@scottlowe
Copy link

Agreed; This is a good idea. I use arel_table an awful lot in order to access predicates, and I find it rather clumsy to have to assign it first.

@oriolgual
Copy link
Contributor

+1 idem as @scottlowe

@guilleiguaran
Copy link
Member

Like this really, +1 (re: https://twitter.com/#!/tenderlove/status/57839406987493376)

@dolzenko
Copy link
Contributor

+1 re https://twitter.com/#!/tenderlove/status/57839406987493376 (with other folks doing slighlty longer relation.table), using for things like .matches and .not_eq where there is some gain compared to using just bare SQL strings)

@txus
Copy link

txus commented Apr 12, 2011

Neat!

@sobrinho
Copy link
Contributor

+1

1 similar comment
@majormajors
Copy link

+1

@eduardordm
Copy link
Contributor

-1, using the class twice does not sound good enough.

@mtodd
Copy link
Contributor

mtodd commented Apr 12, 2011

+1

@jneen
Copy link

jneen commented Apr 12, 2011

Just to throw an idea out there. If it's a DSL, make it a DSL:

famous_awesome_users = User.where { fame.gt(100) & awesome? } # or .where { (fame > 80) & awesome? }

or is that too much magic? :)

@josepjaume
Copy link
Contributor

+1 awesome!

@flavorjones
Copy link
Member

Definitely +1. Makes not-writing SQL more attractive, which is a win for the universe.

@tobi
Copy link

tobi commented Apr 12, 2011

You guys have no sense of what this kind of syntax would do to people new to Ruby. Fit's of despair would probably be a mild side effect. Don't forget, we are not trying to turn Ruby into a gulfing complete version of Perl, we are trying to create a framework which aids in Ruby's main mission of providing syntax that makes sense to people and just incidentally happens to be machine executable. .gt is broderline but something like fame.gt(100) & awesome? is just ridiculous.

in any case -1 from me.

@eduardordm
Copy link
Contributor

famous_users = User.where(User[:fame].gt(80))

Real nice, but repeating User is just WRONG, unless I can do that:

famous_users = User.where(Posts[:age].gt(80))

@grimen
Copy link

grimen commented Apr 12, 2011

My opinion this DSL should be in core:

https://github.com/ernie/meta_where

So much more natural than the one suggested. Would never use the proposed syntax myself, just feels wrong/bloated.

@Informpro
Copy link

tobi > I totally disagree. I just asked 3 people who have any idea in what is even ruby (not a jewel) : "what means for you fame.gt(100) & awesome, knowing that gt means greater than". They all answered me "fame superior to 100 and awesome ?". So I don't think it's a problem, I think the DSL idea makes sense.

And yeah. meta_where is just amazing.

@dpickett
Copy link
Contributor

I'm inclined to agree with @grimen - the syntax just seems off and meta_where is something I frequently use.

I'm not looking forward to explaining the difference between user.attributes[:first_name] and User[:first_name] to newcomers.

I like the idea/concept, but I don't think the DSL is quite right yet.

@ernie
Copy link
Contributor

ernie commented Apr 12, 2011

Thanks for the MetaWhere love but I wouldn't advocate the existing MW go into core, and I wrote it.

Squeel (formerly MW 2.0) plays more nicely with others and the syntax is much improved, as well. It's still a work in progress but I hope to have it wrapped up by RailsConf.

@chanks
Copy link
Contributor

chanks commented Apr 13, 2011

Why not use the Mongoid-style syntax?

User.where(:fame.gt => 80, :awesome => true)

Edit: Never mind, I see that's what meta_where does too.

@ernie
Copy link
Contributor

ernie commented Apr 13, 2011

By way of example, Squeel DSL:

Article.joins{person}.
        where{title.matches('%hello%') & 
              person.name.matches('Ernie%')}.to_sql
=> SELECT "articles".* FROM "articles" 
   INNER JOIN "people" ON "people"."id" = "articles"."person_id"
   WHERE (("articles"."title" LIKE '%hello%' 
           AND "people"."name" LIKE 'Ernie%'))"

Person.joins{children.children.children}.
       where{children.children.children.name =~ 'Joe%'}.to_sql
=> SELECT "people".* FROM "people" 
   INNER JOIN "people" "children_people" 
     ON "children_people"."parent_id" = "people"."id" 
   INNER JOIN "people" "children_people_2" 
     ON "children_people_2"."parent_id" = "children_people"."id"
   INNER JOIN "people" "children_people_3" 
     ON "children_people_3"."parent_id" = "children_people_2"."id" 
   WHERE "children_people_3"."name" LIKE 'Joe%' 

Note.joins{notable(Person)}.
     where{(notable.id == 1) | (id + 3 == notable.id)}.to_sql
=> SELECT "notes".* FROM "notes" 
   INNER JOIN "people" ON "people"."id" = "notes"."notable_id" 
     AND "notes"."notable_type" = 'Person'
   WHERE (("people"."id" = 1 OR "notes"."id" + 3 = "people"."id"))

Person.select{name.op('||', '-diddly').as('flanderized_name')}.
       first.flanderized_name
=> "Aric Smith-diddly" 

Person.where{find_in_set(id, '1,2,3,4')}.to_sql
=> SELECT "people".* FROM "people"  WHERE (find_in_set("people"."id", '1,2,3,4'))

@eduardordm
Copy link
Contributor

ZOMG, -Infinite then.

@tenderlove
Copy link
Member

@ernie has convinced me that this is probably not a good idea. See here. I think for now that if people want fancier syntax, they should use Squeel, so I'm closing this.

Thank you @sikachu for reopening this pull request so that we could get some good community feedback!

❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️

@tenderlove tenderlove closed this Apr 13, 2011
matthewd pushed a commit that referenced this pull request Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet