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 and resolve lint errors #644

Merged
merged 25 commits into from
Oct 22, 2020
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
5e07dcc
Release 1.19.0 (#621)
mi-wood Apr 13, 2020
50dcc64
Release 1.19.1 (#623)
DeeDeeG Jun 10, 2020
6f80471
Release 1.19.2 (#630)
DeeDeeG Jul 8, 2020
493d36d
Release 1.19.3 (#635)
DeeDeeG Aug 19, 2020
2bb9d17
Initialize rubocop
tegandbiscuits Oct 8, 2020
db149b0
Run rubocop with --fix
tegandbiscuits Oct 8, 2020
f6812fa
Don't require magic comment
tegandbiscuits Oct 8, 2020
ac7fa80
Resolve lint errors in `app`
tegandbiscuits Oct 8, 2020
d1b1e42
RSpec linting
tegandbiscuits Oct 8, 2020
3921054
Resolve remaining
tegandbiscuits Oct 8, 2020
c647fad
Fix wrong change
tegandbiscuits Oct 8, 2020
f98b329
Singleton method for verify
tegandbiscuits Oct 9, 2020
1e1d597
New lint rules
tegandbiscuits Oct 9, 2020
a2fd3a1
Add rubocop to travis config
tegandbiscuits Oct 9, 2020
190a77e
Correct docker compose command
tegandbiscuits Oct 9, 2020
e002060
Check for geo
tegandbiscuits Oct 12, 2020
646b474
Fix typo
tegandbiscuits Oct 13, 2020
706a669
Add contributing docs for rubocop
tegandbiscuits Oct 18, 2020
dece788
Merge branch 'master' of https://github.com/RefugeRestrooms/refugeres…
tegandbiscuits Oct 18, 2020
f440d79
Lint from merging master
tegandbiscuits Oct 18, 2020
f65a1bb
Fix bad merge conflict with rails version
tegandbiscuits Oct 18, 2020
56c001d
Merge branch 'develop' of https://github.com/RefugeRestrooms/refugere…
tegandbiscuits Oct 18, 2020
742ff43
Actually fix lock file issues (hopefully)
tegandbiscuits Oct 18, 2020
8d7cee8
Restore lockfiles to state before merges
DeeDeeG Oct 22, 2020
cd7f0b0
spec/support/rspec.rb: Delete duplicate code
DeeDeeG Oct 22, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions .rubocop.yml
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
Copy link
Contributor Author

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.

Copy link
Contributor

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.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 develop branch. Perhaps we should want the quotation mark normalization done separately so it stays a separate commit on develop branch.

Copy link
Contributor

@DeeDeeG DeeDeeG Oct 17, 2020

Choose a reason for hiding this comment

The 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
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ before_script:
- chmod +x ./cc-test-reporter
- ./cc-test-reporter before-build
script:
- docker-compose run -e web rubocop
- docker-compose run -e "RAILS_ENV=test" web rake db:test:prepare spec
after_script:
- ./cc-test-reporter after-build --exit-code $TRAVIS_TEST_RESULT --prefix /refugerestrooms
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ group :development, :test do
gem 'listen', '>= 3.0.5', '< 3.2'
gem 'pry'
gem 'rspec-rails'
gem 'rubocop', require: false
gem 'rubocop-rails', require: false
gem 'rubocop-rspec', require: false
end

group :development do
Expand Down
28 changes: 28 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ GEM
arbre (1.2.1)
activesupport (>= 3.0.0)
arel (9.0.0)
ast (2.4.1)
autoprefixer-rails (9.7.5)
execjs
bcrypt (3.1.13)
Expand Down Expand Up @@ -234,6 +235,9 @@ GEM
nokogiri (1.10.10)
mini_portile2 (~> 2.4.0)
orm_adapter (0.5.0)
parallel (1.19.2)
parser (2.7.2.0)
ast (~> 2.4.1)
pg (1.2.3)
pg_search (2.3.2)
activerecord (>= 5.2)
Expand Down Expand Up @@ -285,6 +289,7 @@ GEM
method_source
rake (>= 0.8.7)
thor (>= 0.19.0, < 2.0)
rainbow (3.0.0)
rake (13.0.1)
rakismet (1.5.4)
ransack (2.3.2)
Expand All @@ -300,6 +305,7 @@ GEM
responders (3.0.0)
actionpack (>= 5.0)
railties (>= 5.0)
rexml (3.2.4)
rspec-core (3.9.1)
rspec-support (~> 3.9.1)
rspec-expectations (3.9.1)
Expand All @@ -317,6 +323,24 @@ GEM
rspec-mocks (~> 3.9)
rspec-support (~> 3.9)
rspec-support (3.9.2)
rubocop (0.92.0)
parallel (~> 1.10)
parser (>= 2.7.1.5)
rainbow (>= 2.2.2, < 4.0)
regexp_parser (>= 1.7)
rexml
rubocop-ast (>= 0.5.0)
ruby-progressbar (~> 1.7)
unicode-display_width (>= 1.4.0, < 2.0)
rubocop-ast (0.7.1)
parser (>= 2.7.1.5)
rubocop-rails (2.8.1)
activesupport (>= 4.2.0)
rack (>= 1.1)
rubocop (>= 0.87.0)
rubocop-rspec (1.43.2)
rubocop (~> 0.87)
ruby-progressbar (1.10.1)
ruby2_keywords (0.0.2)
ruby_dep (1.5.0)
safe_yaml (1.0.5)
Expand Down Expand Up @@ -360,6 +384,7 @@ GEM
thread_safe (~> 0.1)
uglifier (4.2.0)
execjs (>= 0.3.0, < 3)
unicode-display_width (1.7.0)
unicode_utils (1.4.0)
warden (1.2.8)
rack (>= 2.0.6)
Expand Down Expand Up @@ -418,6 +443,9 @@ DEPENDENCIES
rails (= 5.2.4.4)
rakismet
rspec-rails
rubocop
rubocop-rails
rubocop-rspec
sassc-rails
sdoc
simple_form (~> 5.0)
Expand Down
3 changes: 1 addition & 2 deletions app/admin/admin_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@
filter :email

form do |f|
f.inputs "Admin Details" do
f.inputs 'Admin Details' do
f.input :email
f.input :password
f.input :password_confirmation
end
f.actions
end

end
15 changes: 7 additions & 8 deletions app/admin/dashboard.rb
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') }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For anyone unaware, creating a hash like { foo: 'bar' } and { :foo => 'bar' } is the same (=> is only needed if you don't want the key to be a symbol).


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

Expand All @@ -29,5 +28,5 @@
# end
# end
# end
end # content
end
end
5 changes: 2 additions & 3 deletions app/admin/restroom.rb
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
5 changes: 3 additions & 2 deletions app/controllers/api/docs_controller.rb
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
20 changes: 13 additions & 7 deletions app/controllers/api/v1/restrooms.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class Restrooms < Grape::API
version 'v1'
format :json

# rubocop:disable Metrics/BlockLength
resource :restrooms do
desc "Get all restroom records ordered by date descending."
params do
Expand Down Expand Up @@ -49,14 +50,18 @@ class Restrooms < Grape::API
r = r.current
r = r.accessible if params[:ada].present?
r = r.unisex if params[:unisex]
paginate(r.near([params[:lat], params[:lng]], 20, :order => 'distance'))
paginate(r.near([params[:lat], params[:lng]], 20, order: 'distance'))
end

desc "Search for restroom records updated or created on or after a given date"
params do
optional :ada, type: Boolean, desc: "Only return restrooms that are ADA accessible."
optional :unisex, type: Boolean, desc: "Only return restrooms that are unisex."
optional :updated, type: Boolean, desc: "Return restroom records updated (rather than created) since given date"
optional(
:updated,
type: Boolean,
desc: "Return restroom records updated (rather than created) since given date"
)
requires :day, type: Integer, desc: "Day"
requires :month, type: Integer, desc: "Month"
requires :year, type: Integer, desc: "Year"
Expand All @@ -65,16 +70,17 @@ class Restrooms < Grape::API
r = Restroom
r = r.current
date = Date.new(params[:year], params[:month], params[:day])
if params[:updated]
r = r.updated_since(date)
else
r = r.created_since(date)
end
r = if params[:updated]
r.updated_since(date)
else
r.created_since(date)
end
r = r.accessible if params[:ada].present?
r = r.unisex if params[:unisex].present?
paginate(r.order(created_at: :desc))
end
end
# rubocop:enable Metrics/BlockLength
end
end
end
7 changes: 3 additions & 4 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ def mobile_filter_header
@mobile = true
end

def set_locale
I18n.locale = http_accept_language.language_region_compatible_from(I18n.available_locales)
end

def set_locale
I18n.locale = http_accept_language.language_region_compatible_from(I18n.available_locales)
end
end
4 changes: 4 additions & 0 deletions app/controllers/contacts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ def new
@contact = Contact.new(restroom_id: params['restroom_id'], restroom_name: params['restroom_name'])
end

# rubocop:disable Metrics/AbcSize
# rubocop:disable Metrics/MethodLength
def create
@contact = Contact.new(params[:contact])
unless @contact.valid?
Expand All @@ -26,4 +28,6 @@ def create
flash.now[:error] = nil
flash.now[:notice] = I18n.t('contacts.submitted.thank-you-exclamation')
end
# rubocop:enable Metrics/AbcSize
# rubocop:enable Metrics/MethodLength
end
37 changes: 28 additions & 9 deletions app/controllers/restrooms_controller.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
require_relative '../helpers/recaptcha_helper'

# rubocop:disable Metrics/ClassLength
class RestroomsController < ApplicationController
respond_to :html, :json

before_action :list_restrooms, only: [:index]
before_action :find_restroom, only: [:show, :update, :edit, :destroy]
before_action :find_restroom, only: %i[show update edit]

def index
if params[:nearby]
Expand All @@ -14,6 +15,7 @@ def index
end
end

# rubocop:disable Metrics/MethodLength
def new
if params[:edit_id]
@restroom = find_restroom
Expand All @@ -27,7 +29,12 @@ def new
@restroom = Restroom.new
end
end
# rubocop:enable Metrics/MethodLength

def show; end

# rubocop:disable Metrics/AbcSize
# rubocop:disable Metrics/MethodLength
def create
@restroom = Restroom.new(permitted_params)

Expand Down Expand Up @@ -57,12 +64,18 @@ def create
render 'new'
end
end
# rubocop:enable Metrics/AbcSize
# rubocop:enable Metrics/MethodLength

def edit; end

# rubocop:disable Metrics/AbcSize
# rubocop:disable Metrics/MethodLength
def update
if params[:restroom][:downvote]
Restroom.increment_counter(:downvote, @restroom.id)
Restroom.increment_counter(:downvote, @restroom.id) # rubocop:disable Rails/SkipsModelValidations
elsif params[:restroom][:upvote]
Restroom.increment_counter(:upvote, @restroom.id)
Restroom.increment_counter(:upvote, @restroom.id) # rubocop:disable Rails/SkipsModelValidations
elsif @restroom.update(permitted_params)
flash[:notice] = I18n.t('restroom.flash.updated')
else
Expand All @@ -72,20 +85,23 @@ def update

redirect_to @restroom
end
# rubocop:enable Metrics/AbcSize
# rubocop:enable Metrics/MethodLength

private

private
def list_restrooms
@restrooms = Restroom.current.page(params[:page])
@restrooms = if params[:search].present? || params[:map] == "1"
@restrooms.near([params[:lat], params[:long]], 20, :order => 'distance')
else
@restrooms.reverse_order
end
@restrooms.near([params[:lat], params[:long]], 20, order: 'distance')
else
@restrooms.reverse_order
end
end

def display_errors
if @restroom.errors.any?
errors = @restroom.errors.each do |attribute, message|
@restroom.errors.each do
flash[:alert] = I18n.t('restroom.flash.field')
end
else
Expand All @@ -97,6 +113,7 @@ def find_restroom
@restroom = Restroom.find(params[:id])
end

# rubocop:disable Metrics/MethodLength
def permitted_params
params.require(:restroom).permit(
:name,
Expand All @@ -115,4 +132,6 @@ def permitted_params
:approved
)
end
# rubocop:enable Metrics/MethodLength
end
# rubocop:enable Metrics/ClassLength
Loading