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

Remove duplicate settings already configured by default #8

Closed
wants to merge 1 commit into from

Conversation

r7kamura
Copy link
Contributor

@r7kamura r7kamura commented Jan 4, 2024

Summary

Some settings are already set by default on the inherited gems side.

Why not remove duplicate settings in order to draw the reader's attention to the more essential configuration?

Details

Layout/EndAlignment

AutoCorrect: true is configured by default.

Layout/IndentationConsistency

Enabled: false is configured by default.

Also, do not add any settings for the disabled cops.

Layout/IndentationWidth

Enabled: false is configured by default.

Layout/SpaceInsideArrayLiteralBrackets

EnforcedStyleForEmptyBrackets: no_space is configured by default.

Layout/SpaceInsideHashLiteralBraces

EnforcedStyleForEmptyBraces: no_space is configured by default.

Layout/SpaceInsidePercentLiteralDelimiters

Enabled: false is configured by default.

Rails/AssertNot

Include: ["**/test/**/*"] is configured by default.

Also, do not add any settings for the disabled cops.

Rails/RefuteMethods

Include: ["**/test/**/*"] is configured by default.

Also, do not add any settings for the disabled cops.

Style/AndOr

Enabled: false is configured by default.

Style/PercentLiteralDelimiters

PreferredDelimiters: ... is configured by default with the same settings.

Style/RedundantPercentQ

Enabled: false is configured by default.

@r7kamura
Copy link
Contributor Author

r7kamura commented Jan 4, 2024

do not add any settings for the disabled cops.

To be honest, I am not exactly sure if this decision is correct. This is because, for example, rubocop-minitest and rubocop-rails are actually disabled with DisabledByDefault: true even though they are required, so they are not effective as they are.

The fact that they are required means that they are intended to be enabled on the user side, right? So, the act of disabling it but adding the setting may be meaningful.

@bensheldon
Copy link

I added some more detail in #14. My recommendation would be to remove DisabledByDefault: true and instead maintain these explicit enables/disables.

@igor-alexandrov
Copy link

Maybe the idea of these explicit Enabled: false was to guarantee that they will still be disabled in configurations that inherit rubocop-rails-omakase but have DisabledByDefault: false. Just my suggestion.

@jeremy
Copy link
Member

jeremy commented Nov 3, 2024

Maybe the idea of these explicit Enabled: false was to guarantee that they will still be disabled in configurations that inherit rubocop-rails-omakase but have DisabledByDefault: false. Just my suggestion.

Yes. They'll also override preceding config.

The key thing is that presence of the cops communicates intent, regardless of default.

@jeremy jeremy closed this Nov 3, 2024
@r7kamura r7kamura deleted the remove-duplicated-settings branch November 4, 2024 05:42
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.

4 participants