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

Replace references to ActiveRecord::Base with ApplicationRecord #310

Merged
merged 1 commit into from
Feb 6, 2022

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Feb 6, 2022

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thanks!
Looks good with a single exception in "Define Model Class Migrations" guideline.

README.adoc Outdated
@@ -1162,7 +1162,7 @@ end
# same table you had during the creation of the migration.
# In future if you override the `Product` class and change the `table_name`,
# it won't break the migration or cause serious data corruption.
class MigrationProduct < ActiveRecord::Base
Copy link
Member

Choose a reason for hiding this comment

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

I'm on the fence with changing it here, and the reason is #284.
Wondering what enhancements could go into ApplicationRecord that would be useful in migrations.
And since those enhancements are a subject to change over time, running historical migrations can be affected by the same issue as with renaming/removing models.
I'd keep this guideline intact, maybe with a clarification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've put this one back to ActiveRecord::Base.

@andyw8 andyw8 force-pushed the andyw8/use-application-record branch from 3a0975c to 43b7590 Compare February 6, 2022 12:55
@andyw8 andyw8 merged commit ac89109 into rubocop:master Feb 6, 2022
@andyw8 andyw8 deleted the andyw8/use-application-record branch February 6, 2022 12:58
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