Add rubocop linter and CI job #579

murny · 2024-05-30T01:57:46Z

This MR adds Rubcop using StandardRB and other Rubocop addons. This MR fixes all the violations that currently exists and turns on a CI job for rubocop linting going forward.

~~Uses the new style guide for rubocop from Rails: https://github.com/rails/rubocop-rails-omakase~~

~~Which should be a very lightweight style guide. We could later on use the one from Jupiter or go to something like Standard~~

Decided to use Standard instead, via rubocop.yml. As Connor discovered, many places where tab/spacing wasn't done right with rubocop-rails-omakase. So instead of trying to fix and turn on some custom cops that will fix the spacing, decided to just use Standard instead.

Most corrections are the following:

Use double quotes for strings
Spacing (spaces in arrays, 2 spaces for tabs, spacing before assignment, etc)
Don't use hash rockets for hashes
Special comment for frozen string literals on every ruby file

murny · 2024-06-04T17:05:36Z

Gemfile.lock

@@ -307,7 +344,12 @@ GEM
    zeitwerk (2.6.13)

 PLATFORMS
-  ruby


This wasn't working for me with Github Actions. Believe you don't use ruby anymore and instead break it down by individual platforms like so (this is what a newly generated Rails app will do it appears)

murny · 2024-06-04T17:06:44Z

app/controllers/profiles_controller.rb

@@ -1,113 +1,113 @@
 class ProfilesController < ApplicationController
-	$units = {:access => "Access Services", 


Mainly whitespace diffs (2 spaces for tabs vs the previous 4 spaces for tabs). Also use symbols over hash rockets.

ConnorSheremeta

Noticed some indentation issues, Layout/IndentationWidth is disabled in rubocop-rails-omakase (https://github.com/rails/rubocop-rails-omakase/blob/1c1962b59c5135ed2168fd26229224e9a1c53c8e/rubocop.yml#L70) which I suspect is why these are not flagged as rubocop violations. Everything else looks good to me other than that!

ConnorSheremeta · 2024-06-04T19:25:28Z

app/models/profile.rb

+  friendly_id :full_name, use: :slugged
+  def full_name
+      "#{first_name}-#{last_name}"
+    end


Extra indentation here

ConnorSheremeta · 2024-06-04T19:29:15Z

db/migrate/20181101154627_create_profiles.rb

-      	t.string :liason
-      	t.timestamps null: false
+      t.string :first_name
+        t.string :last_name


Extra indentation until line 21

Oh, good catches. Yeah, not a fan of this 🤔.

Wonder if we can just turn that Layout/IndentationWidth cop you found back on as a one off and this problem goes away.

Will try to do this.

ConnorSheremeta · 2024-06-04T20:55:42Z

app/models/profile.rb

-  	end
+  def self.to_csv(options = {})
+      CSV.generate(options) do |csv|
+      csv << column_names


Missing indentation here and below

ConnorSheremeta · 2024-06-04T20:59:06Z

app/controllers/profiles_controller.rb

+      @profiles = @heads + @staff
+      elsif path.include? "building"
+        @building = params[:building]
+      @buildingname = $buildings[params[:building].to_sym]


Missing indentation here and on line below

ConnorSheremeta · 2024-06-04T21:01:50Z

app/controllers/profiles_controller.rb

+    path = request.url
+      if path.include? "unit"
+        @unit = params[:unit]
+      @unitname = $units[params[:unit].to_sym]


Missing indentation here until line 47

ConnorSheremeta · 2024-06-04T21:02:35Z

app/controllers/profiles_controller.rb

+    digrepo: "Digital Repository & Data Services",
+    dsc: "Digital Scholarship Centre",
+    facilities: "Facilities",
+                health: "Faculty Engagement (Health Sciences)",


Extra indentation here and on line 16

ConnorSheremeta · 2024-06-04T21:03:07Z

app/controllers/profiles_controller.rb

+    stratigic: "Strategic Partnerships",
+    tl: "Teaching & Learning",
+    ux: "User Experience" }
+    $buildings = { augustana: "Augustana Campus Library",


Extra indentation here

ConnorSheremeta · 2024-06-04T21:04:23Z

app/controllers/profiles_controller.rb


+  def show
+      @profile = Profile.friendly.find(params[:id])


Extra indentation here along with others up until line 98

murny · 2024-06-14T16:08:55Z

Hey @ConnorSheremeta, Trica said your back today. Need another review here if you can 🙏.

Main difference is I switched to Standard over Rails-omakase, due to those weird spacing issues you discovered.

ConnorSheremeta

Looks good to me! I am however curious about linting being done on CI in a separate job rather than as a step in the main job (which is how jupiter is setup)

library-cms/.github/workflows/ci.yml

Line 9 in 1f16575

lint:

I wonder if the runtime of those checks would differ having a different setup 🤔

ConnorSheremeta · 2024-06-14T21:30:41Z

app/models/profile.rb

-class Profile < ActiveRecord::Base
+# frozen_string_literal: true
+
+class Profile < ApplicationRecord


ConnorSheremeta · 2024-06-14T21:31:01Z

app/models/profile.rb

      csv << column_names
-      all.each do |product|
-          csv << product.attributes.values_at(*column_names)
+      find_each do |product|


ConnorSheremeta · 2024-06-14T21:31:21Z

config/environments/staging.rb

@@ -84,9 +86,9 @@
  # config.logger = ActiveSupport::TaggedLogging.new(Syslog::Logger.new 'app-name')

  if ENV["RAILS_LOG_TO_STDOUT"].present?
-    logger           = ActiveSupport::Logger.new(STDOUT)
+    logger = ActiveSupport::Logger.new($stdout)


ConnorSheremeta · 2024-06-14T21:31:39Z

config/initializers/rollbar.rb

-  config.exception_level_filters.merge!({
-    "ActionController::RoutingError" => "ignore"
-  })
+  config.exception_level_filters["ActionController::RoutingError"] = "ignore"


ConnorSheremeta · 2024-06-14T21:41:47Z

config/environments/production.rb

@@ -83,9 +85,9 @@
  # config.logger = ActiveSupport::TaggedLogging.new(Syslog::Logger.new "app-name")

  if ENV["RAILS_LOG_TO_STDOUT"].present?
-    logger           = ActiveSupport::Logger.new(STDOUT)
+    logger = ActiveSupport::Logger.new($stdout)


murny · 2024-06-17T20:40:56Z

Looks good to me! I am however curious about linting being done on CI in a separate job rather than as a step in the main job (which is how jupiter is setup)

library-cms/.github/workflows/ci.yml

Line 9 in 1f16575

lint:

I wonder if the runtime of those checks would differ having a different setup 🤔

It's still using the same runtime. ruby/setup-ruby@v1 will set up ruby and bundle install using the project's Gemfile (so we are using the same Rubocop version the project is using). So it should be the exactly same thing just with fewer dependencies involved (don't need a DB, JavaScript runtime, etc to run ruby linter).

This also allows the linter job and test job to run parallel together (which might not be big on a small app but in theory means faster CI runs).

Don't really see much concern here, especially when GitHub Actions is free (I think?) to run it parallelized like this.

murny changed the title ~~Add rubocop-rails-omakase, autofix violations and turn on CI job~~ Add rubocop linter and CI job May 30, 2024

murny commented Jun 4, 2024

View reviewed changes

murny marked this pull request as ready for review June 4, 2024 17:08

ConnorSheremeta reviewed Jun 4, 2024

View reviewed changes

murny added 2 commits June 10, 2024 22:18

Add rubocop-rails-omakase, autofix violations and turn on CI job

9223604

Switch to StandardRB rubocop linter

1f16575

murny force-pushed the add-rubocop branch from 94db640 to 1f16575 Compare June 11, 2024 04:19

ConnorSheremeta approved these changes Jun 14, 2024

View reviewed changes

murny merged commit b48ea72 into master Jun 17, 2024
2 checks passed

murny deleted the add-rubocop branch June 17, 2024 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rubocop linter and CI job #579

Add rubocop linter and CI job #579

murny commented May 30, 2024 •

edited

Loading

murny Jun 4, 2024

murny Jun 4, 2024

ConnorSheremeta left a comment

ConnorSheremeta Jun 4, 2024

ConnorSheremeta Jun 4, 2024

murny Jun 5, 2024

ConnorSheremeta Jun 4, 2024

ConnorSheremeta Jun 4, 2024

ConnorSheremeta Jun 4, 2024

ConnorSheremeta Jun 4, 2024

ConnorSheremeta Jun 4, 2024

ConnorSheremeta Jun 4, 2024

murny commented Jun 14, 2024

ConnorSheremeta left a comment •

edited

Loading

ConnorSheremeta Jun 14, 2024

ConnorSheremeta Jun 14, 2024

ConnorSheremeta Jun 14, 2024

ConnorSheremeta Jun 14, 2024

ConnorSheremeta Jun 14, 2024

murny commented Jun 17, 2024

		@@ -1,113 +1,113 @@
		class ProfilesController < ApplicationController
		$units = {:access => "Access Services",

Add rubocop linter and CI job #579

Add rubocop linter and CI job #579

Conversation

murny commented May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConnorSheremeta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murny commented Jun 14, 2024

ConnorSheremeta left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murny commented Jun 17, 2024

murny commented May 30, 2024 •

edited

Loading

ConnorSheremeta left a comment •

edited

Loading