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

Introduce AllCops: MigratedSchemaVersion config #1383

Merged
merged 1 commit into from
Dec 25, 2024

Conversation

koic
Copy link
Member

@koic koic commented Nov 8, 2024

This PR introduces AllCops: MigratedSchemaVersion config.

By specifying MigratedSchemaVersion option, migration files that have been migrated can be ignored. When MigratedSchemaVersion: '20241231000000' is set. Migration files lower than or equal to '20250101000000' will be ignored. For example, this is the timestamp in db/migrate/20250101000000_create_articles.rb.

AllCops
  MigratedSchemaVersion: 20250101000000

This prevents inspecting schema settings for already applied migration files and avoids having different database schemas depending on when bin/rails db:migrate is executed.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.

Copy link
Contributor

@Earlopain Earlopain left a comment

Choose a reason for hiding this comment

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

I like this, much nicer than having to configure it for each cop individually. This makes sense I think, you never want any of these cop to run for already run migrations and potentially change them. cc @fatkodima

README.md Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/usage.adoc Outdated Show resolved Hide resolved
@Earlopain
Copy link
Contributor

Hm, would it be possible to add a hint for MigratedSchemaVersion to the cop message automatically? I think that would be a nice little improvement. If a new cop gets added that looks at migrations there will be many new offenses that should not be fixed.

Configure MigratedSchemaVersion if this migration has already been run.

Or something like that.

This PR introduces `AllCops: MigratedSchemaVersion` config.

By specifying the `MigratedSchemaVersion` option, migration files that have already been run can be ignored.
When `MigratedSchemaVersion: '20241225000000'` is set, migration files lower than or equal to '20241225000000' will be ignored.
For example, to ignore db/migrate/20241225000000_create_articles.rb and earlier migrations you would configure it the following way:

```yaml
AllCops
  MigratedSchemaVersion: '20241225000000'
```

This prevents inspecting schema settings for already applied migration files.
Changing already applied migrations should be avoided because it can lead to the schema getting out of sync
between your local copy and what it actually is in production, depending on when `bin/rails db:migrate` was executed.
If you want to modify your schema to comply with the cops, you should instead create new migrations.
@koic koic force-pushed the intro_migrated_schema_version branch from b2bbf6f to 86b7a55 Compare December 25, 2024 08:34
@koic
Copy link
Member Author

koic commented Dec 25, 2024

As long as it doesn't result in redundant message display, it might be possible to add some sort of hint. In any case, this could be handled as a separate enhancement.

@koic koic merged commit 2c357f0 into rubocop:master Dec 25, 2024
13 of 14 checks passed
@koic koic deleted the intro_migrated_schema_version branch December 25, 2024 08:40
@koic
Copy link
Member Author

koic commented Dec 25, 2024

NOTE: The build error in the JRuby CI matrix can be ignored as it has been reported in ruby/bigdecimal#309.

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