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
```
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.
Having exceptions is better than having silent bugs.
There are a few methods I've kept the same way they were.
The `RelatedContentScore#score_with_opposite` method is a bit peculiar:
it creates scores for both itself and the opposite related content,
which means the opposite related content will try to create the same
scores as well.
We've already got a test to check `Budget::Ballot#add_investment` when
creating a line fails ("Edge case voting a non-elegible investment").
Finally, the method `User#send_oauth_confirmation_instructions` doesn't
update the record when the email address isn't already present, leading
to the test "Try to register with the email of an already existing user,
when an unconfirmed email was provided by oauth" fo fail if we raise an
exception for an invalid user. That's because updating a user's email
doesn't update the database automatically, but instead a confirmation
email is sent.
There are also a few false positives for classes which don't have bang
methods (like the GraphQL classes) or destroying attachments.
For these reasons, I'm adding the rule with a "Refactor" severity,
meaning it's a rule we can break if necessary.
The code `where(id: ids)` is equivalent to `where(id: ids.uniq)`.
Since Rails 5 uses `distinct` instead of `uniq` and in most cases where
we use `uniq` with `pluck` we should simply remove the `uniq` call (as
done in this commit), we're also removing the `Rails/UniqBeforePluck`
rubocop rule.
For performance purposes, we need to find bottlenecks in our
application. Optimizing the performance of small methods doesn't make
the application faster.
I've kept a few cops because applying these ones IMHO make the code
easier to read.
These are rules we were already applying.
We've excluded the `factories` folder for some rules because there's a
factory defining a `context` attribute, which rubocop thought was the
`context` RSpec keyword.
We use staging and preproduction environments, which are not valid by
default.
This rule is useful because misspelling the name of an environment might
otherwise go unnoticed.
Using Date.today and Time.now is a common mistake which might lead to
obscure bugs. Now we're making sure we'll receive a warning when a pull
request uses these methods.
The reason for this rule is similar to dynamic factories: we want
dynamic dates to be evaluted relative to the current time, and not
relative to the moment the application was loaded.
Factory bot has stopped supporting dynamic attributes, and we'll have to
change all factories before upgrading.
In order to apply the rubocop rule, we need to bump rubocop-rspec to its
latest version.
These rules are now defined under `Style/`. However, we don't really use
them, so they add noise to the rubocop configuration file without
solving any issues we actually have.
We forgot to add these changes to pull requests which were in
development before we upgraded to Rails 5.
We're also moving the rubocop rules to the basic files, so we're
notified when we inherit from `ActiveRecord::Base`.
We've agreed `User.new` is easier to read than `described_class.new` and
since we are ignoring Hound's comments regarding this topic, we might as
well remove it.
