-
Notifications
You must be signed in to change notification settings - Fork 260
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 and resolve lint errors #644
Changes from all commits
5e07dcc
50dcc64
6f80471
493d36d
2bb9d17
db149b0
f6812fa
ac7fa80
d1b1e42
3921054
c647fad
f98b329
1e1d597
a2fd3a1
190a77e
e002060
646b474
706a669
dece788
f440d79
f65a1bb
56c001d
742ff43
8d7cee8
cd7f0b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
# The behavior of RuboCop can be controlled via the .rubocop.yml | ||
# configuration file. It makes it possible to enable/disable | ||
# certain cops (checks) and to alter their behavior if they accept | ||
# any parameters. The file can be placed either in your home | ||
# directory or in some project directory. | ||
# | ||
# RuboCop will start looking for the configuration file in the directory | ||
# where the inspected file is and continue its way up to the root directory. | ||
# | ||
# See https://docs.rubocop.org/rubocop/configuration | ||
|
||
require: | ||
- rubocop-rails | ||
- rubocop-rspec | ||
|
||
AllCops: | ||
NewCops: enable | ||
Exclude: | ||
- 'Gemfile' | ||
- '**/*.rake' | ||
- 'bin/**/*' | ||
- 'config/**/*' | ||
- 'db/**/*' | ||
- 'node_modules/**/*' | ||
- 'vendor/**/*' | ||
- 'tmp/**/*' | ||
- 'Rakefile' | ||
- 'config.rb' | ||
- 'config.ru' | ||
|
||
Style/Documentation: | ||
Enabled: false | ||
|
||
Style/FrozenStringLiteralComment: | ||
Enabled: false | ||
|
||
Style/StringLiterals: | ||
Enabled: false | ||
Comment on lines
+37
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the rule for checking that strings use single quotes or double quotes. Right now there's a mixture, so this should probably be enabled for consistency. I turned it off so this pull request touches less things, but I could enable it now or later if desired. The default is single quotes unless it'll make a difference, but I could put it for double. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default sounds good to me. I think double-quote usage should be reserved for when it means something, so as to draw attention to whatever advanced/interpolation stuff is going on in our strings. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Pull Request is already rather large, so I personally think it would be okay to include the quotation mark normalization here as well. If it is committed in a neat way here, that would make it easy to review. However, maybe saving it for a separate Pull Request is good for this reason: We have "squash pull requests" enabled at this repository. So this entire Pull Request is destined to become one commit on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, thanks for the thoughtful approach to enabling this linter. Anticipating some of the pitfalls, however minor, increases my confidence with adding the linter and is making the experience better of adding it to the repository. It's personally my first time using Rubocop, so that's great. |
||
|
||
Metrics/BlockLength: | ||
ExcludedMethods: | ||
- describe | ||
- context | ||
- factory | ||
- define | ||
|
||
RSpec/ExampleLength: | ||
Enabled: false | ||
|
||
RSpec/MultipleExpectations: | ||
Enabled: false | ||
|
||
Style/RegexpLiteral: | ||
AllowInnerSlashes: true |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,14 +81,27 @@ This is equivalent, but slower during a code-test-code-test development cycle: | |
docker-compose run web rspec spec/models/restroom_spec.rb | ||
``` | ||
|
||
### 7 Shut down the Docker Container: | ||
### 7 Linting Code | ||
Ruby code is linted with [rubocop](https://docs.rubocop.org/). | ||
|
||
If you want to lint your code before pushing it, you can run: | ||
``` | ||
docker-compose run web rubocop | ||
``` | ||
|
||
Some lint issues can be resolved automatically by running: | ||
``` | ||
docker-compose run web rubocop --auto-correct | ||
``` | ||
|
||
### 8 Shut down the Docker Container: | ||
Comment on lines
+84
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 thanks for adding documentation. Awesome. ❤️ |
||
In another terminal window, run: | ||
``` | ||
docker-compose down | ||
``` | ||
_(Shutting down the container in this way is safer than exiting with `Ctrl + C`, and prevents issues with breaking the `db` container.)_ | ||
|
||
### 8 Optional tasks: | ||
### 9 Optional tasks: | ||
To clean up encoding problems in the safe2pee data, run (Use `rake db:fix_accents[dry_run]` to preview the changes.): | ||
``` | ||
docker-compose run rake db:fixaccents | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,11 @@ | ||
ActiveAdmin.register_page "Dashboard" do | ||
menu priority: 1, label: proc { I18n.t('active_admin.dashboard') } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For anyone unaware, creating a hash like |
||
|
||
menu :priority => 1, :label => proc{ I18n.t("active_admin.dashboard") } | ||
|
||
content :title => proc{ I18n.t("active_admin.dashboard") } do | ||
div :class => "blank_slate_container", :id => "dashboard_default_message" do | ||
span :class => "blank_slate" do | ||
span I18n.t("active_admin.dashboard_welcome.welcome") | ||
small I18n.t("active_admin.dashboard_welcome.call_to_action") | ||
content title: proc { I18n.t('active_admin.dashboard') } do | ||
div class: 'blank_slate_container', id: 'dashboard_default_message' do | ||
span class: 'blank_slate' do | ||
span I18n.t('active_admin.dashboard_welcome.welcome') | ||
small I18n.t('active_admin.dashboard_welcome.call_to_action') | ||
end | ||
end | ||
|
||
|
@@ -29,5 +28,5 @@ | |
# end | ||
# end | ||
# end | ||
end # content | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
ActiveAdmin.register Restroom do | ||
|
||
permit_params :name, :street, :city, :state, :accessible, :changing_table, :unisex, :directions, | ||
:comment, :latitude, :longitude, :country, :edit_id, :approved | ||
permit_params :name, :street, :city, :state, :accessible, :changing_table, :unisex, :directions, | ||
:comment, :latitude, :longitude, :country, :edit_id, :approved | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
class Api::DocsController < ApplicationController | ||
def index | ||
module Api | ||
class DocsController < ApplicationController | ||
def index; end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this setting if an update to rubocop adds a new rule it'll automatically be enabled, which could cause the test suite to fail linting.
I could remove this setting so it just logs that a rule has been added, but in my experience new rules don't usually cause issues and not automatically enabling causes more work to have to manually enable them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good as-is. I think your reasoning sounds good; I can agree with that approach.