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

ActiveRecord 4.1 compatibility #436

Closed
wants to merge 4 commits into from

Conversation

eagletmt
Copy link
Contributor

This PR fixes #435.

@eagletmt
Copy link
Contributor Author

Oh, it seems to lose AR 3 compatibility... I'll investigate.

In ActiveRecord 3, Model.all returns an Array.
@@ -125,7 +125,7 @@
end

it "should have tag_counts_on" do
TaggableModel.tag_counts_on(:tags).should be_empty
TaggableModel.tag_counts_on(:tags).count(:all).should be_zero
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, so this test doesn't cover the tags.is_a?(Array) scenario...

@eagletmt
Copy link
Contributor Author

I tried another way to fix about count.
What do you think about extending the scope returned from all_tags and all_tag_counts?
https://github.com/eagletmt/acts-as-taggable-on/compare/ar-4.1-compat-2

@@ -2,7 +2,11 @@ module ActsAsTaggableOn
module TagsHelper
# See the README for an example using tag_cloud.
def tag_cloud(tags, classes)
return [] if tags.empty?
if tags.is_a?(Array)
Copy link
Collaborator

Choose a reason for hiding this comment

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

def tag_cloud(tags, classes)
  return [] if no_tags?(tags)
  ....
end

def no_tags?(tags)
  if tags.is_a?(ActiveRecord::Relation)
    tags.count(:all).zero?
  else
    tags.blank?
   end
end

@bf4
Copy link
Collaborator

bf4 commented Dec 27, 2013

See #445 (you'll probably have to rebase once it's merged)

@eagletmt
Copy link
Contributor Author

I opened another PR #446. I believe it's better than #436.

@bf4
Copy link
Collaborator

bf4 commented Dec 27, 2013

Oh, you know you can force push to the same remote branch and it will update the PR?

@bf4
Copy link
Collaborator

bf4 commented Dec 27, 2013

Closed per #446

@bf4 bf4 closed this Dec 27, 2013
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.

Incompatibility with activerecord 4.1.0.beta1
2 participants