-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix aggregates #307
Fix aggregates #307
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple small comments. Overall looks awesome! :D
scores.each { |score| UserBox.create &.average_score(score) } | ||
query_sum = UserQuery.new.average_score.lt(out_of_range).average_score.select_sum | ||
query_sum.should eq sum | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test would still be nice to have but maybe there is a reason for deleting it? Is it covered by another test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's covered in a different test. I created a generic describe "#select_sum" do
that handles the 2 non type specific tests here:
Lines 402 to 415 in 6925948
describe "#select_sum" do | |
it "works with chained where clauses" do | |
UserBox.create &.total_score(2000) | |
UserBox.create &.total_score(1000) | |
UserBox.create &.total_score(3000) | |
sum = UserQuery.new.total_score.gte(2000).total_score.select_sum | |
sum.should eq 5000 | |
end | |
it "returns nil if there are no records" do | |
query_sum = UserQuery.new.age.select_sum | |
query_sum.should be_nil | |
end | |
end |
It seemed unnecessary to repeat for every type of integer and float. Let me know if you would like me to do something a little different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see! I think this was a good call 👍
it "returns 0 if there are no records" do | ||
query_sum = UserQuery.new.id.select_sum! | ||
query_sum.should eq 0_i64 | ||
query_sum.should eq 0 | ||
query_sum.should be_a Int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. More robust to check the type 👍
Looks awesome. Loving these new fixes and improved/more accurate type signatures in aggregates |
Addresses issues with aggregate queries
See #292, #304, and #287