Class variables in Ruby are not the same as instance variables of a
class. They're particularly tricky when it comes to inheritance and
modules.
In the case of the Measurable module, for example, using a class
variable will make all classes including the Measurable module share
the same value. However, that's not what we want; we want the variable
to be different for every class. And that's accomplished using a class
instance variable.
Although in this case it would probably be better to remove the caching
variable. I don't think these methods are called more than once during a
request, and even if they did it's highly unlikely the would become a
bottleneck.
The `and` and `or` keywords are not equivalent to `&&` and `||` and its
use is counterintuitive. Here's an example
```
good = true && false # good if false
bad = true and false # bad is true
```
The reason is `and` and `or` are control flow operators. So the code:
```
bad = true and false
```
Is equivalent to:
```
if bad = true
false
end
```
I had mixed feelings about this rule, since I like spaces where
possible.
However, I changed my mind when I realized writing `->(thing) { }` was
similar to defining a method, and we don't have a space before the
parenthesis when defining a method.
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.
