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.

Sign up for GitHub

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

Jump to bottom

Accept arguments for #count #303

Closed
wants to merge 1 commit into from
Closed

Accept arguments for #count #303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/will_paginate/active_record.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,14 @@ def total_entries
end
end

def count
def count(*args)
if limit_value
excluded = [:order, :limit, :offset, :reorder]
excluded << :includes unless eager_loading?
rel = self.except(*excluded)
# TODO: hack. decide whether to keep
rel = rel.apply_finder_options(@wp_count_options) if defined? @wp_count_options
rel.count
rel.count(*args)
else
super
end
Expand Down
5 changes: 5 additions & 0 deletions spec/finders/active_record_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,11 @@
$query_sql.last.should_not =~ /\ORDER\b/
end

it "accepts arguments for #count" do
Project.page(1).count(distinct: true)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish this test was more solid. It should assert the result of count. The data and the query should be set up in a way that makes count() yield different results depending on whether it was with or without arguments. This way we're sure that the arguments really got forwarded and that the correct SQL query was executed. Then there will be no need for inspecting the query directly on the next line.

Also, if such test passes on CI, we are confident that it works in all combinations of Active Record + db adapter versions.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the end I removed this test before merging, since it didn't yield the same results on all Rails versions that we test with (~ half of them failed)

$query_sql.last.should =~ /COUNT\(DISTINCT/
end

it "should not have zero total_pages when the result set is empty" do
Developer.where("1 = 2").page(1).total_pages.should == 1
end
Expand Down