Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rubocop lint rules #3735

Merged
merged 7 commits into from
Oct 5, 2019
Merged

Add rubocop lint rules #3735

merged 7 commits into from
Oct 5, 2019

Conversation

javierm
Copy link
Member

@javierm javierm commented Sep 28, 2019

Background

The Ruby interpreter issues warnings on code having patterns which could easily result in bugs. Rubocop has rules to detect all those warnings and some more.

Objectives

  • Fix existing warnings which make code confusing
  • Add rubocop rules to prevent future warnings

Notes

There's one Ruby warning I haven't fixed: the autoload trigger in PollsController. We'll have to investigate whether the issue loading classes is still present after upgrading to Rails 5.

There are also several rubocop rules I haven't added; they either give false positives, or are a bit overkill.

@javierm javierm added the Linters Rubocop, ERB Lint, ESLint, SCSS-Lint, ... label Sep 28, 2019
@javierm javierm self-assigned this Sep 28, 2019
@javierm javierm added this to Reviewing in Roadmap via automation Sep 28, 2019
@javierm javierm force-pushed the rubocop_lint branch 2 times, most recently from e077d1b to 9781876 Compare September 29, 2019 22:50
@javierm javierm changed the base branch from remove_unused_variables to rspec_instance_variables September 29, 2019 22:51
@javierm javierm force-pushed the rspec_instance_variables branch 5 times, most recently from cec060e to 4de4045 Compare September 30, 2019 14:43
@javierm javierm changed the base branch from rspec_instance_variables to rubocop_fixes September 30, 2019 14:53
@javierm javierm force-pushed the rubocop_fixes branch 5 times, most recently from 50290d9 to 545f161 Compare October 5, 2019 12:42
Naming two variables the same way is confusing at the very least, and
can lead to hard to debug errors. That's why the Ruby interpreter issues
a warning when we do so.
This is a very subtle behaviour: `match /attachment/i` could represent a
regular expression, but it could also represent a division like
`match / attachment / i`. So we need to make an exception to the usual
way we omit parenthesis in RSpec expectations.
We're using `yield` in the method body.
These methods were defined with `attr_reader` (or accessor in some
cases) and then they were redefined.
There are more cases where we have unused block arguments, but I'm only
changing the obvious ones.
@javierm javierm moved this from Reviewing to Testing in Roadmap Oct 5, 2019
@javierm javierm changed the base branch from rubocop_fixes to master October 5, 2019 13:41
@javierm javierm merged commit 42dbbf5 into master Oct 5, 2019
Roadmap automation moved this from Testing to Release 1.1.0 Oct 5, 2019
@javierm javierm deleted the rubocop_lint branch October 5, 2019 13:50
smarques pushed a commit to venetochevogliamo/consul that referenced this pull request Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linters Rubocop, ERB Lint, ESLint, SCSS-Lint, ...
Projects
No open projects
Roadmap
  
Release 1.1.0
Development

Successfully merging this pull request may close these issues.

None yet

1 participant