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

Add new `Rails/ThreeStateBoolean` cop

Open fatkodima opened this issue 2 years ago • 6 comments

Closes https://github.com/rubocop/rubocop-rails/issues/337

fatkodima avatar Mar 18 '22 12:03 fatkodima

I had just come to the repo to check if this cop was being worked on already so I could submit it. Thank you for doing this!

bbugh avatar Apr 19 '22 16:04 bbugh

@koic Can you merge one of my PR, having StartAfterMigrationVersion in it, like this PR? So I will create new PRs adding this config option to other existing cops (like AddColumnIndex, BulkChangeTable etc) without copying this module over and over.

fatkodima avatar May 06 '22 11:05 fatkodima

Could you separate the StartAfterMigrationVersion config into another PR?

Also, I would like to request the following changes regarding StartAfterMigrationVersion.

  • Rename from StartAfterMigrationVersion to MigratedSchemaVersion
  • It would be better to set it for the Rails department instead of cop units. Only one value has been migrated. Perhaps user does not want to set it individually for each cop.

I think config/default.yml can be null (~) by default.

Rails:
  # Warnings for db/schema.rb are ignored up to the specified schema version.
  # e.g.: MigratedSchemaVersion: '2022_05_05_080819'
  MigratedSchemaVersion: ~

User can configure user's .rubocop.yml as follows:

Rails:
  MigratedSchemaVersion: '202007130158'

It also supports comparison of the following two timestamp formats:

ActiveRecord::Schema.define(version: 20220505080819) do

and

ActiveRecord::Schema.define(version: 2022_05_05_080819) do

This setting can be applied to all existing cop related for schema migration in the future.

koic avatar May 06 '22 12:05 koic

@koic, should I extend Rubocop::Config with MigratedSchemaVersion, like here https://github.com/rubocop/rubocop/blob/0d23a0a72fa83baac48f8da930ac8d72cb1bd2e4/lib/rubocop/config.rb#L222-L231? Or create a mixin and add it to some existing cop (for testing)?

fatkodima avatar May 06 '22 13:05 fatkodima

TBH, I haven't considered the details of how to implement it yet. So, I can take the configuration task later (maybe a few days later) . First of all, it is enough if StartAfterMigrationVersion is dropped from this PR. I will continue to review this PR after that.

koic avatar May 06 '22 13:05 koic

Updated with all the suggestions.

fatkodima avatar May 06 '22 13:05 fatkodima

@koic Any chance this PR can be reviewed and merged to master? If there is anything I can help you or @fatkodima to finish the PR just let me know. Thanks for creating this cop!

denzelem avatar Mar 01 '23 07:03 denzelem

OK. Let's introduce it first.

koic avatar Mar 01 '23 07:03 koic

I think Rails/ThreeStateBooleanColumn is more specific for the cop name. What do you think?

koic avatar Mar 01 '23 07:03 koic

Thanks!

koic avatar Mar 31 '23 15:03 koic

Thanks @koic & @fatkodima 💯

denzelem avatar Apr 12 '23 12:04 denzelem