Add RuboCop linter for Ruby code

Parts of the repository contains Ruby code. Let's add RuboCop to it to ensure we follow a common code style. I let RuboCop fix most of the issues, but things like line length had to be fixed manually. I modernized the Ruby gem's RuboCop config (as it's locked on an older version of RuboCop) and made it consistent with the RuboCop config in the diagnose tests repo. appsignal/diagnose_tests#6
appsignal · Aug 11, 2021 · ca41973 · ca41973
1 parent 6bf7f74
commit ca41973
Show file tree

Hide file tree

Showing 17 changed files with 184 additions and 31 deletions.
diff --git a/.gitignore b/.gitignore
@@ -84,3 +84,4 @@ build/
 packages/nodejs-ext/ext/*.tar.gz
 
 packages/*/test/spec/examples.txt
+tmp
diff --git a/.rubocop.yml b/.rubocop.yml
@@ -0,0 +1,72 @@
+AllCops:
+  DisplayCopNames: true
+  UseCache: true
+  CacheRootDirectory: ./tmp
+  NewCops: enable
+  Exclude:
+    - "node_modules/**/*"
+    - "test/integration/diagnose/**/*"
+
+Style/Documentation:
+  Enabled: false
+
+Style/StringLiterals:
+  EnforcedStyle: double_quotes
+
+Style/StringLiteralsInInterpolation:
+  EnforcedStyle: double_quotes
+
+Style/HashSyntax:
+  EnforcedStyle: hash_rockets
+
+Layout/HashAlignment:
+  EnforcedLastArgumentHashStyle: ignore_implicit
+
+Layout/ArgumentAlignment:
+  EnforcedStyle: with_fixed_indentation
+
+Layout/ParameterAlignment:
+  EnforcedStyle: with_fixed_indentation
+
+Layout/MultilineMethodCallIndentation:
+  EnforcedStyle: indented
+
+Layout/MultilineOperationIndentation:
+  EnforcedStyle: indented
+
+Layout/FirstArrayElementIndentation:
+  EnforcedStyle: consistent
+
+Layout/LineEndStringConcatenationIndentation:
+  EnforcedStyle: indented
+
+Layout/EmptyLineBetweenDefs:
+  AllowAdjacentOneLineDefs: true
+
+Layout/LineLength:
+  Max: 100
+
+Style/Lambda:
+  EnforcedStyle: lambda
+
+Style/WordArray:
+  Enabled: false
+
+Style/FrozenStringLiteralComment:
+  Enabled: true
+
+Style/SymbolArray:
+  EnforcedStyle: brackets
+
+Metrics/AbcSize:
+  Enabled: false
+
+Metrics/BlockLength:
+  Exclude:
+    - Rakefile
+    - "**/spec/*.rb"
+
+Metrics/MethodLength:
+  Max: 25
+  Exclude:
+    - Rakefile
diff --git a/.semaphore/semaphore.yml b/.semaphore/semaphore.yml
@@ -20,6 +20,8 @@ global_job_config:
     value: test
   - name: _PACKAGES_CACHE
     value: v1
+  - name: _BUNDLER_CACHE
+    value: v1
   prologue:
     commands:
     - checkout
@@ -39,6 +41,14 @@ blocks:
   dependencies: []
   task:
     jobs:
+    - name: Ruby Lint (RuboCop)
+      commands:
+      - cache restore $_BUNDLER_CACHE-bundler-$(checksum Gemfile.lock)
+      - bundle config set clean 'true'
+      - bundle config set path .bundle
+      - bundle install --jobs=3 --retry=3
+      - cache store $_BUNDLER_CACHE-bundler-$(checksum Gemfile.lock) .bundle
+      - bundle exec rubocop
     - name: Git Lint (Lintje)
       env_vars:
       - name: LINTJE_VERSION

diff --git a/Gemfile b/Gemfile
@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+source "https://rubygems.org"
+
+gem "rubocop"
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -0,0 +1,32 @@
+GEM
+  remote: https://rubygems.org/
+  specs:
+    ast (2.4.2)
+    parallel (1.20.1)
+    parser (3.0.2.0)
+      ast (~> 2.4.1)
+    rainbow (3.0.0)
+    regexp_parser (2.1.1)
+    rexml (3.2.5)
+    rubocop (1.18.4)
+      parallel (~> 1.10)
+      parser (>= 3.0.0.0)
+      rainbow (>= 2.2.2, < 4.0)
+      regexp_parser (>= 1.8, < 3.0)
+      rexml
+      rubocop-ast (>= 1.8.0, < 2.0)
+      ruby-progressbar (~> 1.7)
+      unicode-display_width (>= 1.4.0, < 3.0)
+    rubocop-ast (1.9.1)
+      parser (>= 3.0.1.1)
+    ruby-progressbar (1.11.0)
+    unicode-display_width (2.0.0)
+
+PLATFORMS
+  x86_64-darwin-20
+
+DEPENDENCIES
+  rubocop
+
+BUNDLED WITH
+   2.2.17
diff --git a/Rakefile b/Rakefile
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require "yaml"
 
 namespace :build_matrix do
@@ -31,8 +33,10 @@ namespace :build_matrix do
                 "commands" => [
                   "mono build",
                   "mono run --package @appsignal/nodejs-ext -- npm run build:ext",
-                  "cache store $_PACKAGES_CACHE-packages-$SEMAPHORE_GIT_SHA-v$NODE_VERSION packages",
-                  "cache store $_PACKAGES_CACHE-install-report-$SEMAPHORE_GIT_SHA-v$NODE_VERSION /tmp/appsignal-*-install.report"
+                  "cache store $_PACKAGES_CACHE-packages-$SEMAPHORE_GIT_SHA-v$NODE_VERSION " \
+                    "packages",
+                  "cache store $_PACKAGES_CACHE-install-report-$SEMAPHORE_GIT_SHA-v$NODE_VERSION " \
+                    "/tmp/appsignal-*-install.report"
                 ]
               )
             ]
@@ -44,18 +48,18 @@ namespace :build_matrix do
         primary_jobs = []
         matrix["packages"].each do |package|
           has_package_tests = package_has_tests? package["path"]
-          unless has_package_tests
-            if skipped_packages.add? package["package"]
-              puts "DEBUG: Skipping Node.js tests for #{package["package"]}: No test files found"
-            end
+          if !has_package_tests && skipped_packages.add?(package["package"])
+            puts "DEBUG: Skipping Node.js tests for #{package["package"]}: No test files found"
           end
 
           package["variations"].each do |variation|
             variation_name = variation.fetch("name")
             dependency_specification = variation["packages"]
             update_package_version_command =
               if dependency_specification
-                packages = dependency_specification.map { |name, version| "#{name}@#{version}" }.join(" ")
+                packages = dependency_specification.map do |name, version|
+                  "#{name}@#{version}"
+                end.join(" ")
                 "npm install #{packages} --save-dev"
               end
 
@@ -133,7 +137,7 @@ def build_semaphore_task(task_hash)
   {
     "name" => task_hash.delete("name") { raise "`name` key not found for task" },
     "dependencies" => [],
-    "task" => task_hash.delete("task") { raise "`task` key not found for task" },
+    "task" => task_hash.delete("task") { raise "`task` key not found for task" }
   }.merge(task_hash)
 end
 
@@ -147,9 +151,7 @@ end
 def package_has_tests?(package)
   test_dir = File.join(package, "src/__tests__")
   # Has a dedicated test dir and it contains files
-  if Dir.exist?(test_dir) && Dir.glob(File.join(test_dir, "**", "*.*s")).any?
-    return true
-  end
+  return true if Dir.exist?(test_dir) && Dir.glob(File.join(test_dir, "**", "*.*s")).any?
 
   Dir.glob(File.join(package, "**/*.test.*s")).any?
 end
diff --git a/build_matrix.yml b/build_matrix.yml
@@ -16,6 +16,8 @@ semaphore: # Default `.semaphore/semaphore.yml` contents
       value: test
     - name: _PACKAGES_CACHE
       value: v1
+    - name: _BUNDLER_CACHE
+      value: v1
     prologue:
       commands:
         - checkout
@@ -36,6 +38,14 @@ semaphore: # Default `.semaphore/semaphore.yml` contents
       dependencies: []
       task:
         jobs:
+        - name: Ruby Lint (RuboCop)
+          commands:
+            - cache restore $_BUNDLER_CACHE-bundler-$(checksum Gemfile.lock)
+            - bundle config set clean 'true'
+            - bundle config set path .bundle
+            - bundle install --jobs=3 --retry=3
+            - cache store $_BUNDLER_CACHE-bundler-$(checksum Gemfile.lock) .bundle
+            - bundle exec rubocop
         - name: Git Lint (Lintje)
           env_vars:
             - name: "LINTJE_VERSION"

diff --git a/packages/express/test/Gemfile b/packages/express/test/Gemfile
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 source "https://rubygems.org"
 
 gem "rspec"
diff --git a/packages/express/test/spec/integration_spec.rb b/packages/express/test/spec/integration_spec.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require "net/http"
 
 EXAMPLE_APP_DIR = File.expand_path(File.join("..", "example"), __dir__)
@@ -13,7 +15,7 @@
 
   describe "/" do
     before do
-      @result = Net::HTTP.get(URI('http://localhost:4010/?foo=bar'))
+      @result = Net::HTTP.get(URI("http://localhost:4010/?foo=bar"))
     end
 
     it "renders the index page" do
@@ -23,13 +25,13 @@
     it "sets the root span's name" do
       log = @app.logs
       expect(/Start root span '(\w+)' in 'web'/.match(log)).to be_truthy, log
-      expect(/Set name 'GET \/' for span '#{$1}'/.match(log)).to be_truthy, log
+      expect(log).to match(%r{Set name 'GET /' for span '#{Regexp.last_match(1)}'})
     end
   end
 
   describe "/dashboard" do
     before do
-      @result = Net::HTTP.get(URI('http://localhost:4010/dashboard?foo=bar'))
+      @result = Net::HTTP.get(URI("http://localhost:4010/dashboard?foo=bar"))
     end
 
     it "renders the page" do
@@ -39,13 +41,13 @@
     it "sets the root span's name" do
       log = @app.logs
       expect(/Start root span '(\w+)' in 'web'/.match(log)).to be_truthy, log
-      expect(/Set name 'GET \/dashboard' for span '#{$1}'/.match(log)).to be_truthy, log
+      expect(log).to match(%r{Set name 'GET /dashboard' for span '#{Regexp.last_match(1)}'})
     end
   end
 
   describe "/admin/dashboard" do
     before do
-      @result = Net::HTTP.get(URI('http://localhost:4010/admin/dashboard?foo=bar'))
+      @result = Net::HTTP.get(URI("http://localhost:4010/admin/dashboard?foo=bar"))
     end
 
     it "renders the page" do
@@ -55,7 +57,7 @@
     it "sets the root span's name" do
       log = @app.logs
       expect(/Start root span '(\w+)' in 'web'/.match(log)).to be_truthy, log
-      expect(/Set name 'GET \/admin\/dashboard' for span '#{$1}'/.match(log)).to be_truthy, log
+      expect(log).to match(%r{Set name 'GET /admin/dashboard' for span '#{Regexp.last_match(1)}'})
     end
   end
 end
diff --git a/packages/express/test/spec/spec_helper.rb b/packages/express/test/spec/spec_helper.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require_relative "../../../../test/integration/support/app_runner"
 
 RSpec.configure do |config|

diff --git a/packages/koa/test/Gemfile b/packages/koa/test/Gemfile
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 source "https://rubygems.org"
 
 gem "rspec"
diff --git a/packages/koa/test/spec/integration_spec.rb b/packages/koa/test/spec/integration_spec.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require "net/http"
 
 EXAMPLE_APP_DIR = File.expand_path(File.join("..", "example"), __dir__)
@@ -11,19 +13,19 @@
   after(:context) { @app.stop }
   after { @app.cleanup }
 
-  describe '/' do
+  describe "/" do
     before do
-      @result = Net::HTTP.get(URI('http://localhost:4010/'))
+      @result = Net::HTTP.get(URI("http://localhost:4010/"))
     end
 
-    it 'renders the index page' do
+    it "renders the index page" do
       expect(@result).to match(/Hello World!/)
     end
 
     it "sets the root span's name" do
       log = @app.logs
       expect(/Start root span '(\w+)' in 'web'/.match(log)).to be_truthy, log
-      expect(%r{Set name 'GET /' for span '#{Regexp.last_match(1)}'}.match(log)).to be_truthy, log
+      expect(log).to match(%r{Set name 'GET /' for span '#{Regexp.last_match(1)}'})
     end
   end
 end
diff --git a/packages/koa/test/spec/spec_helper.rb b/packages/koa/test/spec/spec_helper.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require_relative "../../../../test/integration/support/app_runner"
 
 RSpec.configure do |config|

diff --git a/packages/nextjs/test/Gemfile b/packages/nextjs/test/Gemfile
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 source "https://rubygems.org"
 
 gem "rspec"
Original file line number	Diff line number	Diff line change
Expand Up		@@ -84,3 +84,4 @@ build/
		packages/nodejs-ext/ext/*.tar.gz

		packages/*/test/spec/examples.txt
		tmp