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

Config to ignore old migrations for Rails/ThreeStateBooleanColumn #985

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

toydestroyer
Copy link

@toydestroyer toydestroyer commented Apr 15, 2023

Hello, first-time contributor here 👋

I noticed that the Rails/ThreeStateBooleanColumn cop reports offenses in old migrations. And as mentioned by @mvz in #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:

  • 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.

@toydestroyer toydestroyer changed the title add StartAfter config for ThreeStateBooleanColumn Config to ignore old migrations for Rails/ThreeStateBooleanColumn Apr 15, 2023
@toydestroyer toydestroyer marked this pull request as ready for review April 15, 2023 11:12
@fatkodima
Copy link
Contributor

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

@mvz
Copy link
Contributor

mvz commented Apr 16, 2023

@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?

@fatkodima
Copy link
Contributor

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.

@mvz
Copy link
Contributor

mvz commented Apr 16, 2023

Thanks, @fatkodima, that clears things up.

@toydestroyer
Copy link
Author

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?

@fatkodima
Copy link
Contributor

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

@swrobel
Copy link

swrobel commented Apr 25, 2023

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

@r7kamura
Copy link
Contributor

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)

@fatkodima
Copy link
Contributor

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

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.

5 participants