-
Notifications
You must be signed in to change notification settings - Fork 121
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
Fix for Directly inheriting from ActiveRecord::Migration is not supported #160
Conversation
…ord::Migration is not supported', this error
@@ -9,7 +9,7 @@ def self.up | |||
t.timestamps | |||
end | |||
|
|||
add_index :rates, :rater_id | |||
# add_index :rates, :rater_id |
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.
why is this commented out?
if rails5? | ||
"[#{Rails::VERSION::MAJOR}.#{Rails::VERSION::MINOR}]" | ||
else | ||
"[#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}]" |
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.
is this needed? I saw some of the gems didn't include this for Rails lower than v5 though
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, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/generators/ratyrate/ratyrate_generator.rb
<#160 (comment)>:
> 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
<#160 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD-tAkNEjbAQxgw6R3M5N6i-ilhpdjeEks5uhO2hgaJpZM4V-Ii8>
.
|
Devise didn't include the else condition though
|
My bad, can you add a fix?
…On Thu, 4 Oct 2018, 07:32 Yih Yang, ***@***.***> 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
<#160 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AD-tAmA_hBKKLjg7mm6PAgv_uVycayaGks5uhXNRgaJpZM4V-Ii8>
.
|
@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 |
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, ***@***.***> 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
<#160 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AD-tAkhfszxQNjSNhcmcirsIwC9DfrRnks5uhj2LgaJpZM4V-Ii8>
.
|
Added migration version to handle 'Directly inheriting from ActiveRecord::Migration is not supported" while running rails/rake db:migrate