There were a few changes needed:
* Plugins are now specified through `plugin:` config keyword.
* All plugin gems need to be specified explicitly in Gemfile since they
are no longer dependencies of plugins already specified explicitly.
* All plugin gems need to be updated in other to use the new APIs.
* One cop was renamed.
* New offenses safe to correct were corrected directly with `bundle exec
rubocop -a`.
* New offenses unsafe to correct were added to the TODO configuration
with `bundle exec rubocop --auto-gen-config --auto-gen-only-exclude
--exclude-limit 1400 --no-auto-gen-timestamp`.
This class was originally built to flexibly accept paramters in any order. It also allowed you to specify multiple of the same type of parameter, with the later one overriding the earlier.
This is too flexible and likely to cause mistakes. And besides, we don't use that feature!
- Cop: Rails/RedundantActiveRecordAllMethod
- if receiver is an Active Record object, ".all" can be safely removed
- There are 2 allowed receivers that are listed in the
styleguide file (those are defaults cf. cop documentation).
We are using all suggested extensions already and it's not suggesting
anything at the moment. Using the default value for suggesting
extensions will mean that Rubocop will tell us when there's a new
recommended extension for our code base.
Although it says it supports safe autocorrection, it requires a manual fix. I was going to manually fix the violation, but found it didn't really make the code more readable. So i'm making the call to get rid of it 🔥
I found a plugin that promised to retain comments while sorting, so gave it a try: https://marketplace.visualstudio.com/items?itemName=PascalReitermann93.vscode-yaml-sort
It didn't really save any time, because some comments were still stripped so I had to manually fix it up. Also it reprocesses the yaml and removed other formatting like extra line breaks. So I wouldn't recommend it for this case.
Still, it could be useful for maintaining formatting of a large yaml file like our I18n file.
(to make sorting with a plugin easier)
These settings have been here long enough that I think we can safely say they're accepted. There's no need to have a separate category of contested settings anyway in my opinion.
The Rails/HasAndBelongsToMany rule wants model classes for all
many-to-many relationship tables in the database. But our team thinks
that it's useful to declare has_and_belongs_to_many relationships which
don't require an additional model.
This allows us to run the specs separately to generate the
documentation. It's more efficient this way and the separate swagger doc
file is easier to read.
The engine-specific swagger helper also allows us to simplify the spec
files.
Added an exception to our styleguide because it's intended and useful to
have a complete (lengthy) description of the API in one block.
Rubocop was complaining about too many arguments. But
`ApplicationJob#perform` needs all arguments handled in one call. While
we could allow the `perform` method generally to have more arguments,
there could be other methods called `perform` which should still be
scrutinised. Instead, it seems acceptable to me to have more arguments
as long as they are clearly named as keyword arguments. Rails uses this
a lot to document all options including their default values, for
example in Active Storage. It's better then bundling several arguments
in an undocumented hash just to reduce the number of given arguments.
And once we upgraded to Ruby 3.1, we can clean the method calls up as
well. `call(user: user)` becomes `call(user:)` without repetition.
This detects an additional 639 offenses which I added to the todo file.
From now on, when Dependabot bumps rubocop the build may fail due to new
cops. But that's a great opportunity to utilise those new cops and fix
violations straight away. Otherwise we'll never get to them and lose out
on useful autocorrection, for example for updating deprecated syntax.
We shouldn't use app code in migrations because app code changes while
migrations should always do the same to enable old servers to upgrade.
We have some existing migrations which use ApplicationRecord but I found
that changing them retrospectively breaks them already. So let's leave
them alone.
Rubocop often complains while we think that the code is totally fine. I
would like to achieve the default values one day but there are more
important issues at the moment.
We had an old version under "contested settings" and it looks like some
of them were modified. I hope that our new, separate file will
discourage manual tweaks.
We can include the relaxed rules from a gem as well. Let's see if we
need that complexity one day.
We configured Code Climate to be aware of all open issues instead of
ignoring open issues via the rubocop todo lists. We were hoping that
Code Climate could give us some nice stats like progress if it's aware
of all our open issues. But the graphs don't tell us anything. So it's
better to have one source of truth for our open style issues and that is
our rubocop config in this repository. It contains two todo lists that
document all violations we would like to fix in the future but it's okay
to submit pull requests that don't fix those.
Instead of accepting new style issues in Code Climate, every pull
request with new violations has to add them to the rubocop config. I
hope that this will make rubocop more useful and encourage developers to
reduce code style debt.
