rubocop-rails icon indicating copy to clipboard operation
rubocop-rails copied to clipboard

Config to ignore old migrations for `Rails/ThreeStateBooleanColumn`

Open toydestroyer opened this issue 1 year ago • 9 comments

Hello, first-time contributor here 👋

I noticed that the Rails/ThreeStateBooleanColumn cop reports offenses in old migrations. And as mentioned by @mvz in https://github.com/rubocop/rubocop-rails/discussions/982, fixing these offenses in existing migrations is not practical. Currently, the only way to address this issue is to exclude all offending migrations in the config file, which can add unnecessary noise.

To improve this situation, I propose adding a configuration option for this cop that ignores offenses occurring before a specified migration version. This approach is inspired by the strong_migrations gem. The configuration would look like this:

Rails/ThreeStateBooleanColumn:
  StartAfter: 20230415000000 # 0 by default

I'm open to suggestions for a better name for this configuration option. For now, I've used the name from strong_migrations.

Please let me know if you have any feedback or suggestions for improvement. I'm looking forward to your input and would be happy to make any necessary changes.

Thank you!


Before submitting the PR make sure the following are checked:

  • [x] The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • [x] Wrote good commit messages.
  • [ ] Commit message starts with [Fix #issue-number] (if the related issue exists).
  • [x] Feature branch is up-to-date with master (if not - rebase it).
  • [x] Squashed related commits together.
  • [x] Added tests.
  • [x] Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • [x] 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.

toydestroyer avatar Apr 15 '23 11:04 toydestroyer

I tried to introduce such a config option 2 times (https://github.com/rubocop/rubocop-rails/pull/335 and https://github.com/rubocop/rubocop-rails/pull/277), but these are not yet merged to reuse that logic 😄

fatkodima avatar Apr 16 '23 12:04 fatkodima

@fatkodima I'm a little confused by the discussion in those pull requests. Is the intent for offenses in older migrations to never be fixed, not even by using a later migration to change the schema?

mvz avatar Apr 16 '23 17:04 mvz

These cops are checking migration files when doing their job, so there is no point to do this for older migrations.

It would be nice if schema.rb file would be checked too (kinda look at violations you already introduced into your app before), but this is better done by tools like https://github.com/gregnavis/active_record_doctor.

fatkodima avatar Apr 16 '23 17:04 fatkodima

Thanks, @fatkodima, that clears things up.

mvz avatar Apr 16 '23 18:04 mvz

I tried to introduce such a config option 2 times (#335 and #277), but these are not yet merged to reuse that logic 😄

Are there any specific blockers or issues preventing them from being merged?

Since Rails/ThreeStateBooleanColumn is already in master do you want to open a PR with the StartAfterMigrationVersion mixin and include it in this rule?

toydestroyer avatar Apr 17 '23 01:04 toydestroyer

If this PR will be merged first, I will with no problems update with these changes those other two.

fatkodima avatar Apr 17 '23 09:04 fatkodima

IMHO, StartAfter should apply to all cops that look at migrations. It shouldn't need to be set individually cop-by-cop.

swrobel avatar Apr 25 '23 05:04 swrobel

This may be a bit off topic, but in projects where inspecting old migration files seems pointless, I usually disable the inspections as follows:

# .rubocop.yml
inherit_mode:
  merge:
    - Exclude

AllCops:
  Exclude:
    - "db/migrate/2020*.rb"
    - "db/migrate/2021*.rb"
    - "db/migrate/202201*.rb"
    - "db/migrate/202202*.rb"
    - "db/migrate/202203*.rb"
    - "db/migrate/202204*.rb"
    - "db/migrate/202205*.rb"
    - "db/migrate/202206*.rb"
    - "db/migrate/202207*.rb"
    - "db/migrate/202208*.rb"
    - "db/migrate/202209*.rb"

(In this example, the project here has existed since 2020 and I added this setting on 2022-10-01)

r7kamura avatar Apr 25 '23 21:04 r7kamura

@koic Can you please reconsider this? I am currently in a need for such an option.

fatkodima avatar Mar 28 '24 10:03 fatkodima