Add rubocop (#102)

* Add rubocop and all the things that could be autocorrected
* fix poorly named vars
praise our robot overlords
rubocop happy
Add version
Remove empty block
* Use files_by_mappers_via_expanded_cache in more places

Co-authored-by: Ashley Willard <ashley.willard@gusto.com>

Author: tstannard

.rubocop.yml

@@ -0,0 +1,123 @@
+# 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/**/**
+  TargetRubyVersion: 2.6
+
+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/HashSyntax:
+  Enabled: false
+
+Gemspec/DevelopmentDependencies:
+  Enabled: true
+  EnforcedStyle: gemspec

code_ownership.gemspec

@@ -7,6 +7,7 @@ Gem::Specification.new do |spec|
   spec.description   = 'A gem to help engineering teams declare ownership of code'
   spec.homepage      = 'https://github.com/rubyatscale/code_ownership'
   spec.license       = 'MIT'
+  spec.required_ruby_version = '>= 2.6'
 
   if spec.respond_to?(:metadata)
     spec.metadata['homepage_uri'] = spec.homepage
@@ -31,10 +32,11 @@ Gem::Specification.new do |spec|
   spec.add_dependency 'sorbet-runtime', '>= 0.5.11249'
 
   spec.add_development_dependency 'debug'
+  spec.add_development_dependency 'packwerk'
+  spec.add_development_dependency 'railties'
   spec.add_development_dependency 'rake'
   spec.add_development_dependency 'rspec', '~> 3.0'
+  spec.add_development_dependency 'rubocop'
   spec.add_development_dependency 'sorbet'
   spec.add_development_dependency 'tapioca'
-  spec.add_development_dependency 'packwerk'
-  spec.add_development_dependency 'railties'
 end

lib/code_ownership.rb

@@ -18,7 +18,8 @@
 end
 
 module CodeOwnership
-  extend self
+  module_function
+
   extend T::Sig
   extend T::Helpers
 
@@ -57,6 +58,7 @@ def for_team(team)
       ownership_for_mapper = []
       glob_to_owning_team_map.each do |glob, owning_team|
         next if owning_team != team
+
         ownership_for_mapper << "- #{glob}"
       end
 
@@ -66,7 +68,7 @@ def for_team(team)
         ownership_information += ownership_for_mapper.sort
       end
 
-      ownership_information << ""
+      ownership_information << ''
     end
 
     ownership_information.join("\n")
@@ -84,7 +86,7 @@ def self.remove_file_annotation!(filename)
     params(
       autocorrect: T::Boolean,
       stage_changes: T::Boolean,
-      files: T.nilable(T::Array[String]),
+      files: T.nilable(T::Array[String])
     ).void
   end
   def validate!(
@@ -95,10 +97,10 @@ def validate!(
     Private.load_configuration!
 
     tracked_file_subset = if files
-      files.select{|f| Private.file_tracked?(f)}
-    else
-      Private.tracked_files
-    end
+                            files.select { |f| Private.file_tracked?(f) }
+                          else
+                            Private.tracked_files
+                          end
 
     Private.validate!(files: tracked_file_subset, autocorrect: autocorrect, stage_changes: stage_changes)
   end
@@ -150,7 +152,7 @@ def backtrace_with_ownership(backtrace)
 
       [
         CodeOwnership.for_file(file),
-        file,
+        file
       ]
     end
   end
@@ -161,15 +163,15 @@ def for_class(klass)
     @memoized_values ||= T.let(@memoized_values, T.nilable(T::Hash[String, T.nilable(::CodeTeams::Team)]))
     @memoized_values ||= {}
     # We use key because the memoized value could be `nil`
-    if !@memoized_values.key?(klass.to_s)
+    if @memoized_values.key?(klass.to_s)
+      @memoized_values[klass.to_s]
+    else
       path = Private.path_from_klass(klass)
       return nil if path.nil?
 
       value_to_memoize = for_file(path)
       @memoized_values[klass.to_s] = value_to_memoize
       value_to_memoize
-    else
-      @memoized_values[klass.to_s]
     end
   end
 

lib/code_ownership/cli.rb

@@ -14,7 +14,7 @@ def self.run!(argv)
         for_file(argv)
       elsif command == 'for_team'
         for_team(argv)
-      elsif [nil, "help"].include?(command)
+      elsif [nil, 'help'].include?(command)
         puts <<~USAGE
           Usage: bin/codeownership <subcommand>
 
@@ -27,7 +27,6 @@ def self.run!(argv)
       else
         puts "'#{command}' is not a code_ownership command. See `bin/codeownership help`."
       end
-
     end
 
     def self.validate!(argv)
@@ -53,16 +52,16 @@ def self.validate!(argv)
           exit
         end
       end
-      args = parser.order!(argv) {}
+      args = parser.order!(argv)
       parser.parse!(args)
 
       files = if options[:diff]
-        ENV.fetch('CODEOWNERS_GIT_STAGED_FILES') { `git diff --staged --name-only` }.split("\n").select do |file|
-          File.exist?(file)
-        end
-      else
-        nil
-      end
+                ENV.fetch('CODEOWNERS_GIT_STAGED_FILES') { `git diff --staged --name-only` }.split("\n").select do |file|
+                  File.exist?(file)
+                end
+              else
+                nil
+              end
 
       CodeOwnership.validate!(
         files: files,
@@ -79,7 +78,7 @@ def self.for_file(argv)
       # Long-term, we probably want to use something like `thor` so we don't have to implement logic
       # like this. In the short-term, this is a simple way for us to use the built-in OptionParser
       # while having an ergonomic CLI.
-      files = argv.select { |arg| !arg.start_with?('--') }
+      files = argv.reject { |arg| arg.start_with?('--') }
 
       parser = OptionParser.new do |opts|
         opts.banner = 'Usage: bin/codeownership for_file [options]'
@@ -93,22 +92,22 @@ def self.for_file(argv)
           exit
         end
       end
-      args = parser.order!(argv) {}
+      args = parser.order!(argv)
       parser.parse!(args)
 
       if files.count != 1
-        raise "Please pass in one file. Use `bin/codeownership for_file --help` for more info"
+        raise 'Please pass in one file. Use `bin/codeownership for_file --help` for more info'
       end
-      
+
       team = CodeOwnership.for_file(files.first)
 
-      team_name = team&.name || "Unowned"
-      team_yml = team&.config_yml || "Unowned"
+      team_name = team&.name || 'Unowned'
+      team_yml = team&.config_yml || 'Unowned'
 
       if options[:json]
         json = {
           team_name: team_name,
-          team_yml: team_yml,
+          team_yml: team_yml
         }
 
         puts json.to_json
@@ -121,8 +120,6 @@ def self.for_file(argv)
     end
 
     def self.for_team(argv)
-      options = {}
-
       parser = OptionParser.new do |opts|
         opts.banner = 'Usage: bin/codeownership for_team \'Team Name\''
 
@@ -131,14 +128,14 @@ def self.for_team(argv)
           exit
         end
       end
-      teams = argv.select { |arg| !arg.start_with?('--') }
-      args = parser.order!(argv) {}
+      teams = argv.reject { |arg| arg.start_with?('--') }
+      args = parser.order!(argv)
       parser.parse!(args)
 
       if teams.count != 1
-        raise "Please pass in one team. Use `bin/codeownership for_team --help` for more info"
+        raise 'Please pass in one team. Use `bin/codeownership for_team --help` for more info'
       end
-      
+
       puts CodeOwnership.for_team(teams.first)
     end
 

lib/code_ownership/configuration.rb

@@ -17,8 +17,8 @@ class Configuration < T::Struct
     def self.fetch
       config_hash = YAML.load_file('config/code_ownership.yml')
 
-      if config_hash.key?("require")
-        config_hash["require"].each do |require_directive|
+      if config_hash.key?('require')
+        config_hash['require'].each do |require_directive|
           Private::ExtensionLoader.load(require_directive)
         end
       end

lib/code_ownership/mapper.rb

@@ -29,38 +29,33 @@ def all
     # This should be fast when run with ONE file
     #
     sig do
-      abstract.params(file: String).
-        returns(T.nilable(::CodeTeams::Team))
-    end
-    def map_file_to_owner(file)
+      abstract.params(file: String)
+        .returns(T.nilable(::CodeTeams::Team))
     end
+    def map_file_to_owner(file); end
 
     #
     # This should be fast when run with MANY files
     #
     sig do
-      abstract.params(files: T::Array[String]).
-        returns(T::Hash[String, ::CodeTeams::Team])
-    end
-    def globs_to_owner(files)
+      abstract.params(files: T::Array[String])
+        .returns(T::Hash[String, ::CodeTeams::Team])
     end
+    def globs_to_owner(files); end
 
     #
     # This should be fast when run with MANY files
     #
     sig do
       abstract.params(cache: GlobsToOwningTeamMap, files: T::Array[String]).returns(GlobsToOwningTeamMap)
     end
-    def update_cache(cache, files)
-    end
+    def update_cache(cache, files); end
 
     sig { abstract.returns(String) }
-    def description
-    end
+    def description; end
 
     sig { abstract.void }
-    def bust_caches!
-    end
+    def bust_caches!; end
 
     sig { returns(Private::GlobCache) }
     def self.to_glob_cache
@@ -72,6 +67,7 @@ def self.to_glob_cache
 
         mapped_files.each do |glob, owner|
           next if owner.nil?
+
           glob_to_owner_map_by_mapper_description.fetch(mapper.description)[glob] = owner
         end
       end