Rubocop autocorrect all Ruby files, fail CI if warnings found (#172)

I want to require everyone to properly lint their files before merging to master to reduce bugs. Ideally, we would also automatically run `rubocop --autocorrect` on file save (easily achievable through an editor integration) so all our code has consistent style and conventions.

So, this PR
- updates to and requires the current latest RuboCop
- removed some annoying code metric cops
- turned on a few disabled cops I found useful
- rubocop autocorrected all our Ruby files to fix our current warnings and prevent autocorrect from producing out-of-scope diffs in subsequent PRs
- updated CI to fail builds if there are warning-level offenses detected

Ideally, CI would also fail if there are autocorrectable offenses, even if not warning level, but I couldn't figure out how. I asked in rubocop/rubocop#12702 and will follow up once I get the answer.
I will update our dev docs to make it clear we should be using RuboCop.

.circleci/config.yml

@@ -38,6 +38,10 @@ jobs: # a collection of steps
  name: Bundle Install
  command: bundle check || bundle install
 
+ - run:
+ name: Rubocop autocorrections check
+ command: bundle exec rubocop --fail-level W
+
  # Store bundle cache for Ruby dependencies
  - save_cache:
  key: gracetunes-bundle-v2-{{ checksum "Gemfile.lock" }}

.rubocop.yml

@@ -1,13 +1,33 @@
 require:
  - rubocop-rails
 
-Style/Documentation:
+Rails/SkipsModelValidations:
+ Enabled: false
+Rails/SaveBang:
+ Enabled: true
+Style/CollectionMethods:
+ Enabled: true
+Style/ClassAndModuleChildren:
  Enabled: false
-Style/FrozenStringLiteralComment:
+Style/Documentation:
  Enabled: false
 Style/StringLiterals:
  Enabled: false
+Style/SymbolArray:
+ Enabled: false
+Style/WordArray:
+ Enabled: false
 Layout/SpaceInsideHashLiteralBraces:
  Enabled: false
 Layout/LineLength:
- Max: 100
+ Max: 110
+Metrics/MethodLength:
+ Enabled: false
+Metrics/AbcSize:
+ Enabled: false
+Metrics/CyclomaticComplexity:
+ Enabled: false
+Metrics/BlockLength:
+ Enabled: false
+Metrics/ClassLength:
+ Enabled: false

Gemfile

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 source 'https://rubygems.org'
 
 ruby File.read(".ruby-version").strip
@@ -38,7 +40,7 @@ end
 
 group :development do
  gem 'listen'
- gem 'rubocop', require: false
+ gem 'rubocop', '>=1.60', require: false # install rubocop but don't load it into app memory
  gem 'rubocop-rails', require: false
  gem 'solargraph' # ruby language server, need a plugin for editor/IDE to make use of it
  gem 'spring' # keeps the app running in the background so you don't need to keep rebooting it

Gemfile.lock

@@ -139,13 +139,15 @@ GEM
  thor (>= 0.14, < 2.0)
  js_cookie_rails (2.2.0)
  railties (>= 3.1)
+ json (2.7.1)
  json-schema (4.1.1)
  addressable (>= 2.8)
  jwt (2.7.1)
  kramdown (2.4.0)
  rexml
  kramdown-parser-gfm (1.1.0)
  kramdown (~> 2.0)
+ language_server-protocol (3.17.0.3)
  listen (3.8.0)
  rb-fsevent (~> 0.10, >= 0.10.3)
  rb-inotify (~> 0.9, >= 0.9.10)
@@ -203,8 +205,8 @@ GEM
  omniauth-rails_csrf_protection (1.0.1)
  actionpack (>= 4.2)
  omniauth (~> 2.0)
- parallel (1.23.0)
- parser (3.2.2.4)
+ parallel (1.24.0)
+ parser (3.3.0.5)
  ast (~> 2.4.1)
  racc
  pg (1.5.4)
@@ -268,23 +270,25 @@ GEM
  ffi (~> 1.0)
  rdoc (6.6.0)
  psych (>= 4.0.0)
- regexp_parser (2.8.2)
+ regexp_parser (2.9.0)
  reline (0.4.0)
  io-console (~> 0.5)
  request_store (1.5.1)
  rack (>= 1.4)
  reverse_markdown (2.1.1)
  nokogiri
  rexml (3.2.6)
- rubocop (1.31.0)
+ rubocop (1.60.2)
+ json (~> 2.3)
+ language_server-protocol (>= 3.17.0)
  parallel (~> 1.10)
- parser (>= 3.1.0.0)
+ parser (>= 3.3.0.2)
  rainbow (>= 2.2.2, < 4.0)
  regexp_parser (>= 1.8, < 3.0)
  rexml (>= 3.2.5, < 4.0)
- rubocop-ast (>= 1.18.0, < 2.0)
+ rubocop-ast (>= 1.30.0, < 2.0)
  ruby-progressbar (~> 1.7)
- unicode-display_width (>= 1.4.0, < 3.0)
+ unicode-display_width (>= 2.4.0, < 3.0)
  rubocop-ast (1.30.0)
  parser (>= 3.2.1.0)
  rubocop-rails (2.15.2)
@@ -392,7 +396,7 @@ DEPENDENCIES
  rack-timeout
  rails (= 7.1)
  rails-controller-testing
- rubocop
+ rubocop (>= 1.60)
  rubocop-rails
  sass-rails (>= 5)
  sdoc

Rakefile

@@ -1,7 +1,9 @@
+# frozen_string_literal: true
+
 # Add your own tasks in files placed in lib/tasks ending in .rake,
 # for example lib/tasks/capistrano.rake, and they will automatically be available to Rake.
 
-require File.expand_path('../config/application', __FILE__)
+require File.expand_path('config/application', __dir__)
 require 'rake/testtask'
 
 Rails.application.load_tasks
@@ -10,4 +12,4 @@ Rails.application.load_tasks
 Rake::TestTask.new do |t|
  t.pattern = "test/test_*.rb"
 end
-task :default => :test
+task default: :test

app/controllers/api/api_controller.rb

@@ -1,5 +1,6 @@
-class API::APIController < ActionController::API
+# frozen_string_literal: true
 
+class API::APIController < ActionController::API
  before_action :require_sign_in
 
  DEFAULT_PAGE_SIZE = 100
@@ -19,16 +20,16 @@ def require_delete_privileges
  def current_user
  return @current_user if @current_user
 
- if [:user_email, :name, :role].all? { |field| session.key?(field) }
- @current_user = User.new(email: session[:user_email], name: session[:name], role: session[:role])
- end
+ return unless [:user_email, :name, :role].all? { |field| session.key?(field) }
+
+ @current_user = User.new(email: session[:user_email], name: session[:name], role: session[:role])
  end
 
  def render_form_errors(message, errors)
- render json: {message: message, errors: errors}, status: :bad_request
+ render json: {message:, errors:}, status: :bad_request
  end
 
  def render_paginated_result(data, matching_count, total_count)
- render json: {data: data, matching_count: matching_count, total_count: total_count}
+ render json: {data:, matching_count:, total_count:}
  end
 end

app/controllers/api/audits_controller.rb

@@ -29,7 +29,6 @@ def songs_history_index
  song_name = song.name
  end
  [audit, song_name, is_deleted]
-
  end
 
  render_paginated_result(audits_info_list, matching_audits_count, Audited.audit_class.count)

app/controllers/api/songs_controller.rb

@@ -1,5 +1,6 @@
-class API::SongsController < API::APIController
+# frozen_string_literal: true
 
+class API::SongsController < API::APIController
  before_action :require_edit_privileges, only: [:create, :update]
  before_action :require_delete_privileges, only: [:destroy]
 
@@ -19,7 +20,6 @@ def show
  Song.increment_counter(:view_count, song.id, touch: false)
 
  render json: song
-
  end
 
  def index
@@ -82,12 +82,10 @@ def destroy
  end
  end
 
-
-
  private
 
  def song_params
  params.require(:song)
  .permit(:name, :key, :artist, :tempo, :bpm, :standard_scan, :chord_sheet, :spotify_uri)
  end
-end
+end

app/controllers/application_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class ApplicationController < ActionController::Base
  # Prevent CSRF attacks by raising an exception.
  # For APIs, you may want to use :null_session instead.
@@ -11,9 +13,9 @@ class ApplicationController < ActionController::Base
  def current_user
  return @current_user if @current_user
 
- if [:user_email, :name, :role].all? { |field| session.key?(field) }
- @current_user = User.new(email: session[:user_email], name: session[:name], role: session[:role])
- end
+ return unless [:user_email, :name, :role].all? { |field| session.key?(field) }
+
+ @current_user = User.new(email: session[:user_email], name: session[:name], role: session[:role])
  end
 
  def require_sign_in
@@ -29,6 +31,7 @@ def require_edit_privileges
 
  def require_delete_privileges
  return if current_user.can_delete?
+
  flash[:error] = "You don't have deleting privileges."
  redirect_to songs_path
  end

app/controllers/audits_controller.rb

@@ -1,5 +1,6 @@
-class AuditsController < ApplicationController
+# frozen_string_literal: true
 
+class AuditsController < ApplicationController
  DEFAULT_PAGE_SIZE = 20
  private_constant :DEFAULT_PAGE_SIZE
 

app/controllers/general_controller.rb

@@ -1,9 +1,7 @@
-class GeneralController < ApplicationController
-
- def about
- end
+# frozen_string_literal: true
 
- def request_song
- end
+class GeneralController < ApplicationController
+ def about; end
 
-end
+ def request_song; end
+end

app/controllers/sessions_controller.rb

@@ -1,5 +1,6 @@
-class SessionsController < ApplicationController
+# frozen_string_literal: true
 
+class SessionsController < ApplicationController
  skip_before_action :require_sign_in
 
  def new
@@ -15,9 +16,9 @@ def create
  email = email.gsub('acts2.network', 'gpmail.org')
 
  # if person has never signed into GraceTunes before, create a user for him
- unless (@current_user = User.find_by_email(email))
+ unless (@current_user = User.find_by(email:))
  full_name = user_info["name"].split('(')[0].strip # remove churchplant extention
- @current_user = User.create(email: email, name: full_name, role: Role::READER)
+ @current_user = User.create!(email:, name: full_name, role: Role::READER)
  logger.info "New user created: #{@current_user}"
  end
 
@@ -34,14 +35,14 @@ def destroy
 
  def error
  logger.info "Error authenticating user: #{params[:message]}"
- case params[:message]
- when 'invalid_credentials'
- flash[:error] = "Invalid credentials: you must sign in with a Acts2Network account."
- when 'access_denied'
- flash[:error] = "Access to the account was denied."
- else
- flash[:error] = "Something went wrong while authenticating. Please try again."
- end
+ flash[:error] = case params[:message]
+  when 'invalid_credentials'
+   "Invalid credentials: you must sign in with a Acts2Network account."
+  when 'access_denied'
+   "Access to the account was denied."
+  else
+   "Something went wrong while authenticating. Please try again."
+  end
  redirect_to sign_in_path
  end
 end

app/controllers/songs_controller.rb

@@ -1,5 +1,6 @@
-class SongsController < ApplicationController
+# frozen_string_literal: true
 
+class SongsController < ApplicationController
  before_action :require_edit_privileges, only: [:new, :create, :edit, :update]
  before_action :require_delete_privileges, only: [:destroy]
 
@@ -20,17 +21,16 @@ def index
  records_filtered = songs.count
  songs = songs.select('id, artist, tempo, key, name, chord_sheet, spotify_uri')
 
-
  # reorder
  songs = case params[:sort]
- when 'Newest First'
- songs.reorder(created_at: :desc)
- when 'Most Popular First'
- songs.reorder(view_count: :desc)
- else
- # does nothing if search_by_keywords was run, in which case songs are already ordered by relevance
- songs.order(name: :asc)
- end
+  when 'Newest First'
+  songs.reorder(created_at: :desc)
+  when 'Most Popular First'
+  songs.reorder(view_count: :desc)
+  else
+  # does nothing if search_by_keywords was run, in which case songs are already ordered by relevance
+  songs.order(name: :asc)
+  end
 
  # paginate
  if params[:start].present?
@@ -67,7 +67,7 @@ def show
  Formatter.format_song_nashville(@song)
  end
 
- @has_been_edited = @song.audits.updates.count > 0
+ @has_been_edited = @song.audits.updates.count.positive?
 
  respond_to do |format|
  format.html do
@@ -132,8 +132,9 @@ def print
  end
 
  private
+
  def song_params
  params.require(:song)
- .permit(:name, :key, :artist, :tempo, :bpm, :standard_scan, :chord_sheet, :spotify_uri)
+  .permit(:name, :key, :artist, :tempo, :bpm, :standard_scan, :chord_sheet, :spotify_uri)
  end
 end

app/helpers/application_helper.rb

@@ -1,5 +1,6 @@
-module ApplicationHelper
+# frozen_string_literal: true
 
+module ApplicationHelper
  def set_page_title(page_title)
  content_for :title, page_title
  end