wazery / ratyrate

:star: A Ruby Gem that wraps the functionality of jQuery Raty library, and provides optional IMDB style rating.
http://ratingmoviestore.herokuapp.com
237 stars 120 forks source link

Fix for Directly inheriting from ActiveRecord::Migration is not supported #160

Closed an-nasir closed 5 years ago

an-nasir commented 6 years ago

Added migration version to handle 'Directly inheriting from ActiveRecord::Migration is not supported" while running rails/rake db:migrate

an-nasir commented 5 years ago

In else case it'll remain to same versions. This is how it is done in devise BTW

On Wed, 3 Oct 2018, 22:02 Yih Yang, notifications@github.com wrote:

@yihyang commented on this pull request.

In lib/generators/ratyrate/ratyrate_generator.rb https://github.com/wazery/ratyrate/pull/160#discussion_r222389616:

end +

  • def rails5?
  • Rails.version.start_with? '5'
  • end
  • def migration_version
  • if rails5?
  • "[#{Rails::VERSION::MAJOR}.#{Rails::VERSION::MINOR}]"
  • else
  • "[#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}]"

is this needed? I saw some of the gems didn't include this for Rails lower than v5 though

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wazery/ratyrate/pull/160#pullrequestreview-161265031, or mute the thread https://github.com/notifications/unsubscribe-auth/AD-tAkNEjbAQxgw6R3M5N6i-ilhpdjeEks5uhO2hgaJpZM4V-Ii8 .

yihyang commented 5 years ago

Devise didn't include the else condition though

https://github.com/plataformatec/devise/blob/master/lib/generators/active_record/devise_generator.rb#L94-L98

     def migration_version
       if rails5_and_up?
         "[#{Rails::VERSION::MAJOR}.#{Rails::VERSION::MINOR}]"
       end
     end
an-nasir commented 5 years ago

My bad, can you add a fix?

On Thu, 4 Oct 2018, 07:32 Yih Yang, notifications@github.com wrote:

Devise didn't include the else condition though

https://github.com/plataformatec/devise/blob/master/lib/generators/active_record/devise_generator.rb#L94-L98

 def migration_version
   if rails5_and_up?
     "[#{Rails::VERSION::MAJOR}.#{Rails::VERSION::MINOR}]"
   end
 end

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wazery/ratyrate/pull/160#issuecomment-426865558, or mute the thread https://github.com/notifications/unsubscribe-auth/AD-tAmA_hBKKLjg7mm6PAgv_uVycayaGks5uhXNRgaJpZM4V-Ii8 .

yihyang commented 5 years ago

@an-nasir I can't make changes the branch on your repo though. Is it ok if I create another PR or do you want to update it

an-nasir commented 5 years ago

I would love if you can do, lemme know if there is any issues arise on merging with master

On Thu, 4 Oct 2018, 21:55 Yih Yang, notifications@github.com wrote:

@an-nasir https://github.com/an-nasir I can't make changes the branch on your repo though. Is it ok if I create another PR or do you want to update it

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wazery/ratyrate/pull/160#issuecomment-427092778, or mute the thread https://github.com/notifications/unsubscribe-auth/AD-tAkhfszxQNjSNhcmcirsIwC9DfrRnks5uhj2LgaJpZM4V-Ii8 .

yihyang commented 5 years ago

@an-nasir I have created a new PR: https://github.com/wazery/ratyrate/pull/162

yihyang commented 5 years ago

@an-nasir I'm closing this PR as it has been fixed in https://github.com/wazery/ratyrate/pull/162 as we have discussed