Count to Counts with MultiValue and Count fixed

[kediboregi]

so i can use count without group by and counts with group by

[drujensen]

@unequaled86 Thanks for the contribution.

Can you provide some details as to why you want this change? I have never seen counts used before and doesn't really roll off the tongue for me.

Also, its kinda frowned on to have two methods do the same thing. Please change my mind. ;-)

nm, It looks like counts is returning multiple values based on the group by clause. Is this correct?

[kediboregi]

page = 2
limit = 10

 # its add_aggregate_field for group by
posts.where(:title, :like, "%#{search_text}")

 # (already not working with where bc db.query_one)
count = posts.count.run

# pagination
posts.limit(limit)
posts.offset((page - 1)*limit)
results = posts.select

{"results" => results, "pages_size" => count.fdiv(limit).ceil.to_i, "posts_size" => count.to_i}.to_json

in that rest api scenario
it was giving error because of group by clause on count with db.query_one, unable to use with
where. also i need single value count with where method without group by
i created counts its fixed version of old count
deleted group_by from count
so we can use count for that scenario without and group by counts with group by
fast reasonable solution

maybe we can change count method with additional parameters like a add group_by to sql and always multivalue in the future

[drujensen]

@unequaled86 ok. Thanks for the explanation.

I'm not a fan of the counts. I recommend finding a way to get count to work with and without a group_by.

Is this something you would be interested in looking into?

[kediboregi]

@drujensen rewrited and its look like acceptable

[drujensen]

Group By should not be automatically added to the count. Only if a user specifies one should it be included. Let's just remove it for now.

[drujensen]

Much better! I prefer group_by over group but let me know what you think.

[Blacksmoke16]

Minor, but you should run crystal tool format as well. Standard is to use spaces vs tabs for indentation.

[drujensen]

hmm, I see what you mean. Rails uses order and group_by. I think it's ok to follow their lead.

[kediboregi]

i run crystal tool format before commit
i can change it to group_by

[Blacksmoke16]

i run crystal tool format before commit

Perfect, could you update the tabs to use spaces? Guess it doesn't do that for you.

[kediboregi]

i run crystal tool format before commit

Perfect, could you update the tabs to use spaces? Guess it doesn't do that for you.

i did now

[drujensen]

nice work. update the documentation and add a couple more specs for group_by. maybe include a test for chaining multiple group_by as well. Post.group_by(:name).group_by(:age)

[drujensen]

💯

[drujensen]

@unequaled86 hate to do this to you but we merged a large breaking change #346 that is causing issues in this PR. Can you take a look?

[kediboregi]

fixed, please merge it before happen new issues .d

[drujensen]

@unequaled86 Thank you