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

Use major + minor AR versions in 'Directly inheriting' error message #41131

Closed
wants to merge 1 commit into from

Conversation

radar
Copy link
Contributor

@radar radar commented Jan 15, 2021

The error message when inheriting directly from ActiveRecord::Migration had two things wrong with it:

  1. It mentioned the Rails release, instead of the Active Record release.
  2. The version number was hard-coded to 4.2.

Let's use the current version of active record for this error.

"\n" \
" class #{subclass} < ActiveRecord::Migration[4.2]"
" class #{subclass} < ActiveRecord::Migration[#{major}.#{minor}]"
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to keep 4.2 here. That was the only version that didn't have the version subclasses and migrations that still are using direct inheritance were probably created in 4.2.

But I'm not sure, new migrations being written right now would also see the same exception and in that case #{major}.#{minor} makes sense.

It would be good to have a way to differentiate, but I don't think we have and there is more harm in using 6.1 migration to code that was written in 4.2 than using 4.2 for code just being written.

What do you think?

@radar
Copy link
Contributor Author

radar commented Jan 15, 2021 via email

@rafaelfranca
Copy link
Member

True, this will be on Rails 7.0, so all the old migrations that should be using 4.2 are already using it.

I'll merge and update the test monday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants