Merge basic and standard rubocop files in one file #3799
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
javierm
changed the title
javierm
changed the title
javierm
force-pushed
the
inverse_of
branch
10 times, most recently
from
43aab84
to
1da0fe1
Compare
javierm
force-pushed
the
rubocop_all
branch
6 times, most recently
from
db3b277
to
ff16674
Compare
We were trying to test a before_validation call, but the `touch` method skips validations.
javierm
force-pushed
the
rubocop_all
branch
5 times, most recently
from
0775941
to
ca6234d
Compare
The `belongs_to` method already has that option, so there's no need to do it manually in an `after_save` callback.
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).
We don't use Marshal objects in our application.
As mentioned in commit 9d566a2, 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 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.
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.
Hound now supports rubocop 0.75.
smarques
pushed a commit
to venetochevogliamo/consul
that referenced
this pull request
Apr 29, 2020
Merge basic and standard rubocop files in one file
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objectives
Rails/SkipsModelValidationsand apply it to our needsNotes
The rule
Rails/HasManyOrHasOneDependentshould be applied to existing code. I haven't done it because it was the only rule remaining and applying it will require a lot of thought.Running
rubocop --fail-level convention --display-only-fail-level-offenseswill not report any offenses.