A bit of history in order to understand this change.
A year ago we introduced Hound so it would review our pull requests and
warn contributors when they weren't following our coding style.
However, back then we had many rules we hadn't reviewed yet, and we
weren't sure we wanted to ask contributors to follow them.
So we decided to split these files: .rubocop_basic.yml would contain
rules we had already agreed on and wanted contributors to respect, and
.rubocop.yml would contain rules we still had to review.
Now we've finally gone through all these rules. We've removed some of
them, kept some of them, added new ones, and applied them.
Now all rules with a severity level of "convention" or higher return no
offenses. The rules with "severity: refactor" return some offenses,
though:
* Metrics/LineLenght can only be properly accomplished when we define
better multi-line indentation standards, while in some cases long lines
indicate we need to refactor the code
* Rails/DynamicFindBy returns a few false positives
* Rails/HasManyOrHasOneDependent should by all means be implemented,
although it will not be a trivial task
* Rails/SaveBang returns a few false positives and there are a couple of
places where we skip it on purpose
There are also rules excluding some files:
* Rails/InverseOf returns a false positive
* Rails/OutputSafety is ignored on purpose when we add auto-links to a
text we trust
* RSpec/InstanceVariable returns false positives in two files
Other than that, everything works as expected.
This is actually a hack. We want Hound to warn us about this rule;
however, we don't want to be notified about our many existing offenses.
Ideally we would add the `dependent` option to all existing models.
However, this is extra tricky because adding this option would change
existing behavior.
As mentioned in commit 9d566a22, I've kept these performance cops but
not for performance reasons but because they make the code easier to
read.
As for the security cops, we've never had problems with any of them, but
since we recently added an `eval` call by accident, there's a chance we
could do the same with JSONLoad and YAMLLoad.
This method is ambiguous. Sometimes we use it to set invalid data in
tests (which can usually be done with `update_column`), and other times
we use it instead of `update!`.
I'm removing it because, even if sometimes it could make sense to use
it, it's too similar to `update_attributes` (which is an alias for
`update` and runs validations), making it confusing.
However, there's one case where we're still using it: in the
ActsAsParanoidAliases module, we need to invoke the callbacks, which
`update_column` skips, but tests related to translations fail if we use
`update!`. The reason for this is the tests check what happens if we
restore a record without restoring its translations. But that will make
the record invalid, since there's a validation rule checking it has at
least one translation.
I'm not blacklisting any other method which skips validations because we
know they skip validations and use them anyway (hopefully with care).
Not doing so has a few gotchas when working with relations, particularly
with records which are not stored in the database.
I'm excluding the related content file because it's got a very peculiar
relationship with itself: the `has_one :opposite_related_content` has no
inverse; the relation itself is its inverse. It's a false positive since
the inverse condition is true:
```
content.opposite_related_content.opposite_related_content.object_id ==
content.object_id
```
Usually when we specify a `belongs_to` relations, we also specify its
equivalent `has_many`. That allows us to write, for example:
`topic.user.topics`.
Just like we do in the Budget module, and in some places in the Poll and
Legislation modules, we don't need to specify the class name when the
name of the relation matches the name of a class in the same module.
We're going to change CKEditor to an inline editor, and the "ckeditor"
gem doesn't provide an option to do so.
Since using `cktext_area` would automatically generate a "classic"
iframe CKEditor, we need to use `text_area` and load the editor using
JavaScript. Personally I prefer this option anyway.
Note in the jQuery selector we need to use `textarea.html-area`; using
just `.html-area` would fail if there's an error message associated to
the textarea, since Rails will add the `.html-area` class to the error
message.
We were already applying the FindEach rule since commit cae210c1, while
the EnumUniqueness rule is essential when we use `enum`, and the
EnvironmentComparison rule is very useful since it forces us to use a
format the UnknownEnv rule will recognize.
While I don't use this feature, there are developers who do. It's useful
when running migrations and changing branches.
I'm raising an `ActiveRecord::IrreversibleMigration` exception in every
`drop_table` migration because these migrations were all done before
version 1.0.0, and so making all of them reversible would be too much
work for little benefit.
These are rules we can't apply to existing migrations, so I'm excluding
migrations in the affected years from this check. However, we should
probably add timestamps columns and default values in non-null columns
to at least some of the tables which don't have them.
We were using the `timestamps` method most of the time, but sometimes we
were creating the columns manually.
Note editing past migrations if fine as long as the SQL they generate
remains identical, which is the case here.
There are some rules which only affect migration files, and we cannot
enable them if we're excluding those files from being inspected.
We're also changing migrations related to the Rails/TimeZone rule
slightly because these fields were already changed afterwards, so we
aren't changing the schema.
This comment isn't necessary since Ruby 2.0, where UTF-8 became the
default encoding.
I've found this issue thanks to the EmptyLineAfterMagicComment rubocop
rule.
We were inconsistent on this one. I consider it particularly useful when
a method starts with a `return` statement.
In other cases, we probably shouldn't have a guard rule in the middle of
a method in any case, but that's a different refactoring.
We were very inconsistent regarding these rules.
Personally I prefer no empty lines around blocks, clases, etc... as
recommended by the Ruby style guide [1], and they're the default values
in rubocop, so those are the settings I'm applying.
The exception is the `private` access modifier, since we were leaving
empty lines around it most of the time. That's the default rubocop rule
as well. Personally I don't have a strong preference about this one.
[1] https://rubystyle.guide/#empty-lines-around-bodies
We were already using `find_by` most of the time.
Since there are false positives related to our `find_by_slug_or_id!` and
`find_by_manger_login` methods, which cannot be replaced with `find_by`,
I'm adding it indicating the "refactor" severity.
In Ruby, the Kernel class defined the `open` method, which is available
for (almost) every object. So creating a scope with the name `open`
generates a warning indicating we are overwriting the existing `open`
method.
While this warning is pretty much harmless and we could ignore it, it
generates a lot of noise in the logs. So I'm "undefining" the method
before generating the scope, so we don't get the warning all the time.
It looks like we get this warning if we check the dialog message. Using
`accept_confirm` the same way we do in the rest of the application
solves the problem.
