Conversation
|
Very small comments. Otherwise looks awesome! For the count I think we should have a different method otherwise the return type will be weird. How about “select_counts_by_group” or “select_group_counts”? |
|
Can be added in a separate PR though. This is useful even without a count method! |
paulcsmith
left a comment
There was a problem hiding this comment.
Specs look good and overall LGTM. Just retying to understand the result of group_by because it looks the same as without group.
| users = UserQuery.new.group(&.age).group(&.id) | ||
| users.query.statement.should eq "SELECT #{User::COLUMN_SQL} FROM users GROUP BY users.age, users.id" | ||
| users.map(&.name).should contain "Dwight" | ||
| users.map(&.name).should eq ["Dwight", "Michael", "Jim"] |
There was a problem hiding this comment.
Oh so does it just return an array? What does group by do in this case. I thought it'd be a hash or something
There was a problem hiding this comment.
It returns an array here of .map(), but UserQuery.new.group(&.age).group(&.id) just returns the BaseQuery object like normal. It is confusing though because technically in this query, the group is useless.
I see this more as building blocks for the next parts to come as well as the underlying access. But if you have suggestions, or whatever, I'm all ears. Postgres grouping hurts my head lol
|
Yeah let’s merge it! I think this is a good starting point |
Fixes #193
User.group(:status).count #=> {"active" => 100, "inactive" => 4}We have
select_countcurrently, but maybe that needs to become aware if there's some grouping?