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

Update to latest rubocop gems; simplify rubocop.yml #58

Merged
merged 1 commit into from
Jul 15, 2020

Conversation

mattbrictson
Copy link
Contributor

By default, rubocop makes you explicitly opt in or out of new rules that are added in each release. Otherwise it loudly prints warnings to the console. This is annoying and makes updating gems tedious. In practice, we almost always end up enabling the new rules.

This commit updates our rubocop config with:

NewCops: enable

This automatically opts us into any new rules that are added. That means there is no need to explicitly enable individual rules, and so our rubocop config becomes much smaller.

This commit updates rubocop and rubocop-performance to their latest versions and thus enables all the new rules that have been added in those versions. The only rule that caused violations (and that I have disabled), is Style/AccessorGrouping.

By default, rubocop makes you explicitly opt in or out of new rules that
are added in each release. Otherwise it loudly prints warnings to the
console. This is annoying and makes updating gems tedious. In practice,
we almost always end up enabling the new rules.

This commit updates our rubocop config with:

```
NewCops: enable
```

This automatically opts us into any new rules that are added. That means
there is no need to explicitly enable individual rules, and so our
rubocop config becomes much smaller.

This commit updates rubocop and rubocop-performance to their latest
versions and thus enables all the new rules that have been added in
those versions. The only rule that caused violations (and that I have
disabled), is `Style/AccessorGrouping`.
@@ -4,6 +4,7 @@ require:
AllCops:
DisplayCopNames: true
DisplayStyleGuide: true
NewCops: enable
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This automatically opts us into new rules that are added in future versions of rubocop. So we no longer need to explicitly list the new cops with Enabled: true every time we update the gems.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Comment on lines -33 to -34
Layout/LineLength:
Max: 120
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now the default for rubocop so we don't have to explicitly specify it.

Comment on lines -52 to -53
Lint/StructNewOverride:
Enabled: true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of NewCops: enable, we no longer need to explicitly opt in to all these rules.

Comment on lines +73 to +74
Style/AccessorGrouping:
Enabled: false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one caused violations in this repo so I turned it off. Enabling it would mean we would lose our ability to add comments for each individual accessor:

attr_accessor :one # Comment describing :one
attr_accessor :two # Comment describing :two

Rubocop wanted:

attr_accessor :one, :two

@@ -4,6 +4,7 @@ require:
AllCops:
DisplayCopNames: true
DisplayStyleGuide: true
NewCops: enable
TargetRubyVersion: 2.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that this was also unnecessary... that it would default to whatever ruby version is run.

See rubocop/rubocop#5610.

Should we let the version autodetect instead? It's one fewer places to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that TargetRubyVersion can be inferred for app repos, because an app is deployed with a specific version of ruby (usually declared in the Gemfile). But for library code like socrates, we support multiple versions and so that inference can't be done. So we need to be explicit about the lowest version of ruby that this library supports.

If I remove the TargetRubyVersion line, rubocop defaults to 2.7. It is not smart enough to infer based on the required_ruby_version in the gemspec.

Copy link
Contributor

@christiannelson christiannelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@christiannelson
Copy link
Contributor

christiannelson commented Jul 15, 2020 via email

@mattbrictson mattbrictson merged commit f8c039d into master Jul 15, 2020
@mattbrictson mattbrictson deleted the chores/update-rubocop branch July 15, 2020 18:54
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.

2 participants