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

replaced ActsAsTaggableOnSteroids by ActsAsTaggableOn #99

Merged
merged 7 commits into from
Jan 9, 2016

Conversation

rueckerl
Copy link
Contributor

I tried to get rid of the ActsAsTaggableOnSteroids as i got some errors related to it.

I replaced it by ActsAsTaggableOn which is quite similar.
The main advantage is, that now it is a gem and not a copied library that is used.

Changes include:

  • remove of lib/acts_as_taggable_on_steroids/*
  • adding acts-as-taggable-on to Gemfile
  • removed all references to Tag or TagList and replaced them by using the gems versions
  • added the required database migrations
  • modified admin form to edit posts to have a correctly prefilled tag_list input

@rueckerl
Copy link
Contributor Author

I am sorry I could not fix the build error with ruby1.9.3.
This seems to be specific to the test setup not able to install one of the gems. I cannot reproduce this error on my machine, so I cannot give any details on what might be the problem.... Anyways I did not change anything concerning this gem, so I wonder why it worked before....

@gaelian
Copy link
Collaborator

gaelian commented Dec 19, 2015

I think doing something about lib/acts_as_taggable_on_steroids is probably long overdue.

I haven't had a chance to dig into this too much yet, but just a hunch: the Travis build error where it's complaining that for Ruby 1.9.3 "An error occurred while installing rspec-activemodel-mocks (1.0.1)" could be related to a temporary fix (4d0cd84) that's still present in the codebase and I think should be able to be removed now and replaced with the official rspec-activemodel-mocks 1.0.2. Or this could just be a weird coincidence.

The other issue that occurs to me is related to existing users. If this commit drops the existing tag-related tables, then that potentially nukes existing user's tags. A smooth upgrade path of some kind would be preferable here, IMO. @xaviershay, any thoughts?

I won't have time to look further into this for maybe a couple of weeks. But if anyone else wants to take on further investigation, please do.

@rueckerl
Copy link
Contributor Author

Thank you for the reply, @gaelian.

I will look into these issues during the next days.
If the first one is only a replacement of the version number it should easily be fixed.

Concerning the second issue: I will think about a way to fix this. As those two modules are closely related it should be easy to transfer old data by not dropping and recreating the tables. So i will most probably rewrite the migrations into one that updates the tables instead of recreating them.

@rueckerl
Copy link
Contributor Author

I tried to address the two issues and it seems as if I have fixed them.

It would be a great thing if anyone with real data could try the migration once (on a COPY of the real data!!!) to confirm it really does what it should. Using some test data everything seemed fine.

gaelian added a commit that referenced this pull request Jan 9, 2016
Replaced ActsAsTaggableOnSteroids with ActsAsTaggableOn.
@gaelian gaelian merged commit 5e97796 into xaviershay:master Jan 9, 2016
@gaelian
Copy link
Collaborator

gaelian commented Jan 9, 2016

@rueckerl thanks for the contribution! 👍

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.

2 participants