Add rubocop (#20)

* add rubocop

* initial rubocop autocorrect

* more manual fixes

* add linting

.github/workflows/ci.yml

@@ -41,3 +41,16 @@ jobs:
           gem install bundler
           bundle install --jobs 4 --retry 3
           bundle exec srb tc
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@5126516654c75f76bca1de45dd82a3006d8890f9
+      - name: Set up Ruby
+        uses: ruby/setup-ruby@bd94d6a504586da892a5753afdd1480096ed30df
+        with:
+          ruby-version: head
+      - name: Run style checks
+        run: |
+          gem install bundler
+          bundle install --jobs 4 --retry 3
+          bundle exec rubocop

.rubocop.yml

@@ -0,0 +1,118 @@
+# 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
+AllCops:
+  NewCops: enable
+  Exclude:
+    - vendor/bundle/**/**
+
+Metrics/ParameterLists:
+  Enabled: false
+
+# This cop is annoying with typed configuration
+Style/TrivialAccessors:
+  Enabled: false
+
+# This rubocop is annoying when we use interfaces a lot
+Lint/UnusedMethodArgument:
+  Enabled: false
+
+Gemspec/RequireMFA:
+  Enabled: false
+
+Lint/DuplicateBranch:
+  Enabled: false
+
+# If is sometimes easier to think about than unless sometimes
+Style/NegatedIf:
+  Enabled: false
+
+# Disabling for now until it's clearer why we want this
+Style/FrozenStringLiteralComment:
+  Enabled: false
+
+# It's nice to be able to read the condition first before reading the code within the condition
+Style/GuardClause:
+  Enabled: false
+
+#
+# Leaving length metrics to human judgment for now
+#
+Metrics/ModuleLength:
+  Enabled: false
+
+Layout/LineLength:
+  Enabled: false
+
+Metrics/BlockLength:
+  Enabled: false
+
+Metrics/MethodLength:
+  Enabled: false
+
+Metrics/AbcSize:
+  Enabled: false
+
+Metrics/ClassLength:
+  Enabled: false
+
+# This doesn't feel useful
+Metrics/CyclomaticComplexity:
+  Enabled: false
+
+# This doesn't feel useful
+Metrics/PerceivedComplexity:
+  Enabled: false
+
+# It's nice to be able to read the condition first before reading the code within the condition
+Style/IfUnlessModifier:
+  Enabled: false
+
+# This leads to code that is not very readable at times (very long lines)
+Style/ConditionalAssignment:
+  Enabled: false
+
+# For now, we prefer to lean on clean method signatures as documentation. We may change this later.
+Style/Documentation:
+  Enabled: false
+
+# Sometimes we leave comments in empty else statements intentionally
+Style/EmptyElse:
+  Enabled: false
+
+# Sometimes we want to more explicitly list out a condition
+Style/RedundantCondition:
+  Enabled: false
+
+# This leads to code that is not very readable at times (very long lines)
+Layout/MultilineMethodCallIndentation:
+  Enabled: false
+
+# Blocks across lines are okay sometimes
+Style/BlockDelimiters:
+  Enabled: false
+
+# Sometimes we like methods like `get_packages`
+Naming/AccessorMethodName:
+  Enabled: false
+
+# This leads to code that is not very readable at times (very long lines)
+Layout/FirstArgumentIndentation:
+  Enabled: false
+
+# This leads to code that is not very readable at times (very long lines)
+Layout/ArgumentAlignment:
+  Enabled: false
+
+Style/AccessorGrouping:
+  Enabled: false
+
+Style/NumericPredicate:
+  Enabled: false

Gemfile.lock

@@ -85,6 +85,8 @@ GEM
       unicode-display_width (>= 1.4.0, < 3.0)
     rubocop-ast (1.21.0)
       parser (>= 3.1.1.0)
+    rubocop-rspec (2.13.2)
+      rubocop (~> 1.33)
     rubocop-sorbet (0.6.11)
       rubocop (>= 0.90.0)
     ruby-progressbar (1.11.0)
@@ -136,6 +138,8 @@ DEPENDENCIES
   pry-byebug
   rake
   rspec (~> 3.0)
+  rubocop
+  rubocop-rspec
   sorbet
   sorbet-static
   tapioca

lib/use_packwerk.rb

@@ -24,11 +24,11 @@
 module UsePackwerk
   extend T::Sig
 
-  PERMITTED_PACK_LOCATIONS = T.let([
-    'gems',
-    'components',
-    'packs',
-  ], T::Array[String])
+  PERMITTED_PACK_LOCATIONS = T.let(%w[
+                                     gems
+                                     components
+                                     packs
+                                   ], T::Array[String])
 
   sig do
     params(
@@ -45,15 +45,15 @@ def self.create_pack!(
     Private.create_pack!(
       pack_name: pack_name,
       enforce_privacy: enforce_privacy,
-      enforce_dependencies: enforce_dependencies, 
+      enforce_dependencies: enforce_dependencies
     )
   end
 
   sig do
     params(
       pack_name: String,
       paths_relative_to_root: T::Array[String],
-      per_file_processors: T::Array[PerFileProcessorInterface],
+      per_file_processors: T::Array[PerFileProcessorInterface]
     ).void
   end
   def self.move_to_pack!(
@@ -74,7 +74,7 @@ def self.move_to_pack!(
     Private.move_to_pack!(
       pack_name: pack_name,
       paths_relative_to_root: paths_relative_to_root,
-      per_file_processors: per_file_processors,
+      per_file_processors: per_file_processors
     )
 
     Logging.section('Next steps') do
@@ -99,7 +99,7 @@ def self.move_to_pack!(
   sig do
     params(
       paths_relative_to_root: T::Array[String],
-      per_file_processors: T::Array[PerFileProcessorInterface],
+      per_file_processors: T::Array[PerFileProcessorInterface]
     ).void
   end
   def self.make_public!(
@@ -117,7 +117,7 @@ def self.make_public!(
       paths_relative_to_root: paths_relative_to_root,
       per_file_processors: per_file_processors
     )
-  
+
     Logging.section('Next steps') do
       next_steps = <<~NEXT_STEPS
         Your next steps might be:
@@ -174,7 +174,7 @@ def self.add_dependency!(
     params(
       pack_name: String,
       parent_name: String,
-      per_file_processors: T::Array[PerFileProcessorInterface],
+      per_file_processors: T::Array[PerFileProcessorInterface]
     ).void
   end
   def self.move_to_parent!(
@@ -195,7 +195,7 @@ def self.move_to_parent!(
     Private.move_to_parent!(
       pack_name: pack_name,
       parent_name: parent_name,
-      per_file_processors: per_file_processors,
+      per_file_processors: per_file_processors
     )
 
     Logging.section('Next steps') do
@@ -214,7 +214,7 @@ def self.move_to_parent!(
   sig do
     params(
       pack_name: T.nilable(String),
-      limit: Integer,
+      limit: Integer
     ).void
   end
   def self.list_top_privacy_violations(
@@ -247,14 +247,14 @@ def self.list_top_dependency_violations(
     params(
       file: String,
       find: Pathname,
-      replace_with: Pathname,
+      replace_with: Pathname
     ).void
   end
   def self.replace_in_file(file:, find:, replace_with:)
     Private.replace_in_file(
       file: file,
       find: find,
-      replace_with: replace_with,
+      replace_with: replace_with
     )
   end
 end

lib/use_packwerk/cli.rb

@@ -6,13 +6,13 @@ module UsePackwerk
   class CLI < Thor
     extend T::Sig
 
-    desc "create packs/your_pack", "Create pack with name packs/your_pack"
+    desc 'create packs/your_pack', 'Create pack with name packs/your_pack'
     sig { params(pack_name: String).void }
     def create(pack_name)
       UsePackwerk.create_pack!(pack_name: pack_name)
     end
 
-    desc "add_dependency packs/from_pack packs/to_pack", "Add packs/to_pack to packs/from_pack/package.yml list of dependencies"
+    desc 'add_dependency packs/from_pack packs/to_pack', 'Add packs/to_pack to packs/from_pack/package.yml list of dependencies'
     long_desc <<~LONG_DESC
       Use this to add a dependency between packs.
 
@@ -29,7 +29,7 @@ def add_dependency(from_pack, to_pack)
       )
     end
 
-    desc "list_top_dependency_violations packs/your_pack", "List the top dependency violations of packs/your_pack"
+    desc 'list_top_dependency_violations packs/your_pack', 'List the top dependency violations of packs/your_pack'
     option :limit, type: :numeric, default: 10, aliases: :l, banner: 'Specify the limit of constants to analyze'
     sig { params(pack_name: String).void }
     def list_top_dependency_violations(pack_name)
@@ -39,7 +39,7 @@ def list_top_dependency_violations(pack_name)
       )
     end
 
-    desc "list_top_privacy_violations packs/your_pack", "List the top privacy violations of packs/your_pack"
+    desc 'list_top_privacy_violations packs/your_pack', 'List the top privacy violations of packs/your_pack'
     option :limit, type: :numeric, default: 10, aliases: :l, banner: 'Specify the limit of constants to analyze'
     sig { params(pack_name: String).void }
     def list_top_privacy_violations(pack_name)
@@ -49,32 +49,32 @@ def list_top_privacy_violations(pack_name)
       )
     end
 
-    desc "make_public path/to/file.rb path/to/directory", "Pass in a space-separated list of file or directory paths to make public"
+    desc 'make_public path/to/file.rb path/to/directory', 'Pass in a space-separated list of file or directory paths to make public'
     sig { params(paths: String).void }
     def make_public(*paths)
       UsePackwerk.make_public!(
         paths_relative_to_root: paths,
-        per_file_processors: [UsePackwerk::RubocopPostProcessor.new, UsePackwerk::CodeOwnershipPostProcessor.new],
+        per_file_processors: [UsePackwerk::RubocopPostProcessor.new, UsePackwerk::CodeOwnershipPostProcessor.new]
       )
     end
 
-    desc "move packs/destination_pack path/to/file.rb path/to/directory", "Pass in a destination pack and a space-separated list of file or directory paths to move to the destination pack"
+    desc 'move packs/destination_pack path/to/file.rb path/to/directory', 'Pass in a destination pack and a space-separated list of file or directory paths to move to the destination pack'
     sig { params(pack_name: String, paths: String).void }
     def move(pack_name, *paths)
       UsePackwerk.move_to_pack!(
         pack_name: pack_name,
         paths_relative_to_root: paths,
-        per_file_processors: [UsePackwerk::RubocopPostProcessor.new, UsePackwerk::CodeOwnershipPostProcessor.new],
+        per_file_processors: [UsePackwerk::RubocopPostProcessor.new, UsePackwerk::CodeOwnershipPostProcessor.new]
       )
     end
 
-    desc "move_to_parent packs/parent_pack packs/child_pack", "Pass in a parent pack and another pack to be made as a child to the parent pack!"
+    desc 'move_to_parent packs/parent_pack packs/child_pack', 'Pass in a parent pack and another pack to be made as a child to the parent pack!'
     sig { params(parent_name: String, pack_name: String).void }
     def move_to_parent(parent_name, pack_name)
       UsePackwerk.move_to_parent!(
         parent_name: parent_name,
         pack_name: pack_name,
-        per_file_processors: [UsePackwerk::RubocopPostProcessor.new, UsePackwerk::CodeOwnershipPostProcessor.new],
+        per_file_processors: [UsePackwerk::RubocopPostProcessor.new, UsePackwerk::CodeOwnershipPostProcessor.new]
       )
     end
   end

lib/use_packwerk/code_ownership_post_processor.rb

@@ -22,7 +22,7 @@ def before_move_file!(file_move_operation)
         UsePackwerk.replace_in_file(
           file: code_owners_allow_list_file.to_s,
           find: relative_path_to_origin,
-          replace_with: relative_path_to_destination,
+          replace_with: relative_path_to_destination
         )
       end
 
@@ -45,11 +45,11 @@ def print_final_message!
       if @teams.any?
         Logging.section('Code Ownership') do
           Logging.print('This section contains info about the current ownership distribution of the moved files.')
-          @teams.group_by { |team| team }.sort_by { |team, instances| -instances.count }.each do |team, instances|
+          @teams.group_by { |team| team }.sort_by { |_team, instances| -instances.count }.each do |team, instances|
             Logging.print "  #{team} - #{instances.count} files"
           end
           if @did_move_files
-            Logging.print "Since the destination package has package-based ownership, file-annotations were removed from moved files."
+            Logging.print 'Since the destination package has package-based ownership, file-annotations were removed from moved files.'
           end
         end
       end

lib/use_packwerk/configuration.rb

@@ -13,22 +13,22 @@ class Configuration
     sig { void }
     def initialize
       @enforce_dependencies = T.let(@enforce_dependencies, T.nilable(T::Boolean))
-      @documentation_link = T.let(documentation_link, T.nilable(String) )
+      @documentation_link = T.let(documentation_link, T.nilable(String))
     end
 
     sig { returns(T::Boolean) }
     def enforce_dependencies
-      if !@enforce_dependencies.nil?
-        @enforce_dependencies
-      else
+      if @enforce_dependencies.nil?
         true
+      else
+        @enforce_dependencies
       end
     end
 
     # Configure a link to show up for users who are looking for more info
     sig { returns(String) }
     def documentation_link
-      "https://go/packwerk"
+      'https://go/packwerk'
     end
   end
 
@@ -42,7 +42,7 @@ def config
     end
 
     sig { params(blk: T.proc.params(arg0: Configuration).void).void }
-    def configure(&blk) # rubocop:disable Lint/UnusedMethodArgument
+    def configure(&blk)
       yield(config)
     end
   end