Disable Rails/PluckId cop by default

[mjankowski]

This cop was originally disabled with the original rubocop import: 71e48d5#diff-da3213a24b350fa386ff3bab0d6b239112408c3fe4f986cbd9d5236f7e0b61e4R458-R463

It was subsequently enabled during a mass enable commit: 24bc50f#diff-da3213a24b350fa386ff3bab0d6b239112408c3fe4f986cbd9d5236f7e0b61e4R248-R249

As discussed on this issue, the cop is unsafe by default and frequently gets false positives: rubocop/rubocop-rails#301

It is disabled by default in most recent rubocop release, for same reasons: https://github.com/rubocop/rubocop-rails/blob/v2.25.0/config/default.yml#L746-L751

Discussion of why to disable on: #31

[searls]

@mjankowski if you have a moment -- could you scan thru the rubocop-rails to make sure we're not enabling anything that's disabled-by-default over there?

[mjankowski]

Sure thing, quick summary here. Here are the cops which have Enabled: false in rubocop-rails (as of their main branch today), and their corresponding values in standard-rails…

• ActionFilter — true
• DefaultScope — false
• EnvironmentVariableAccess — true
• OrderById — true
• PluckId — false (as of this PR)
• RequireDependency — false
• ReversibleMigrationMethodDefinition — true
• SaveBang — true
• SchemaComment — false
• TableNameAssignment — false
• UnusedIgnoredColumns — true

That’s more than I expected that are disabled in the rubocop-rails default config, but enabled in standard-rails. Let me know if any of the following would be useful:

• If you want a sort of default policy that anything which is disabled in rubocop-rails should also be disabled in standard-rails (ie, s-r is a subset of rc-r, or at most the same) I could do a PR to disable more of these … and maybe add test coverage that would help identify any disabled-there-enabled-here gaps that show up during version bumps?
• If it should be more case by case, I could open one issue per cop (looks like 6 cases to consider) with the history of it being enabled in s-r, why it's disabled in rc-r (if I can figure that out quickly) and sort through them all that way?

[mjankowski]

Originally discussed here - #36 (comment)

I did a little more digging through the cops referenced in that previous comment RE: the cops which are disabled by default in rubocop-rails but current enabled in standard-rails. Summary...

Rails/SaveBang

Discussed elsewhere, PR open to disable - #37

Rails/ActionFilter

Standard Rails history: was false in original import, enabled in the bulk enable commit.

This cop is actually deprecated since it detects something from older versions of Rails no longer supported by rubocop - rubocop/rubocop-rails#1149 ... probably makes sense to also disable in S-R?

Rails/EnvironmentVariableAccess

Voted yes during railsconf talk, enabled after that. Upon further review, there is a rubocop-rails side TODO that this will switch to pending in future release. Probably makes sense to keep enabled.

Rails/OrderById

Standard-Rails history: false in original import, enabled in bulk enable commit.

Disabled in rubocop-rails since introduction - rubocop/rubocop-rails#323 - concerns about legitimate cases of wanting to order by ID. Might make sense to disable.

Rails/ReversibleMigrationMethodDefinition

S-R history: false in original import, enabled in bulk enable

Disabled on rubocop-rails side since introduction - rubocop/rubocop-rails#457 - no real explanation/discussion about why.

Rails/UnusedIgnoredColumns

Voted yes during railsconf talk

The rubocop-rails disable was only ~2 months ago, explanation in rubocop/rubocop-rails#1252 - gist of it is possible diffs between dev/prod DBs creating issues.

[searls]

Wow thanks @mjankowski! I'd support disabling all these but Rails/EnvironmentVariableAccess, I guess, then

[mjankowski]

Cool - I'll do individual PRs and try to include full rationale/context.

[mjankowski]

From that original list, these have all been disabled in merged PRs -- SaveBang, ActionFilter, UnusedIgnoredColumns -- with links to rationable of why they were disabled on rubocop-rails side.

There's also an open PR to disable OrderById

We decided to leave EnvironmentVariableAccess enabled.

That leaves ReversibleMigrationDefinition as the last remaining "disabled by default in rubocop-rails but currently enabled in standard-rails" cop. I haven't opened PR for that one because I can't really find an explanation of why it's disabled by default (other than maybe a convention of doing that in initial PR when first proposing a new cop) ... and what it does (check for either a change method or up+down methods in migrations) seems mostly useful to me.

[searls]

I agree on ReversibleMigrationDefinition, it seems like a smart thing to include until/unless we know anyone has been burned by it. @koic, do you think Rails/ReversibleMigrationDefinition should be on-by-default in rubocop-rails?