From 07c898b9dc88727cb50936890719d3363ea7f214 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 16 Dec 2025 12:46:04 -0800 Subject: [PATCH] Remove use of module_function --- .rubocop.yml | 4 +++ code_ownership.gemspec | 2 +- lib/code_ownership.rb | 29 ++++++++----------- lib/code_ownership/cli.rb | 1 + .../private/file_path_finder.rb | 10 ++----- .../private/file_path_team_cache.rb | 14 ++++----- .../private/for_file_output_builder.rb | 1 - lib/code_ownership/private/team_finder.rb | 22 +++++--------- lib/code_ownership/version.rb | 2 +- sorbet/config | 4 +-- .../private/file_path_team_cache_spec.rb | 4 +++ 11 files changed, 40 insertions(+), 53 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 20bbeeb..6f345ad 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -88,6 +88,10 @@ Style/Documentation: Style/EmptyElse: Enabled: false +# This interacts poorly with typed code +Style/ModuleFunction: + EnforcedStyle: forbidden + # Sometimes we want to more explicitly list out a condition Style/RedundantCondition: Enabled: false diff --git a/code_ownership.gemspec b/code_ownership.gemspec index 5ca7a6f..282940c 100644 --- a/code_ownership.gemspec +++ b/code_ownership.gemspec @@ -37,7 +37,7 @@ Gem::Specification.new do |spec| spec.add_dependency 'code_teams', '~> 1.0' spec.add_dependency 'packs-specification' - spec.add_dependency 'sorbet-runtime', '>= 0.5.11249' + spec.add_dependency 'sorbet-runtime', '>= 0.6.12763' spec.add_development_dependency 'debug' spec.add_development_dependency 'packwerk' diff --git a/lib/code_ownership.rb b/lib/code_ownership.rb index 0dc2adb..38c7ae4 100644 --- a/lib/code_ownership.rb +++ b/lib/code_ownership.rb @@ -1,5 +1,4 @@ # frozen_string_literal: true - # typed: strict require 'code_teams' @@ -25,17 +24,13 @@ end module CodeOwnership - module_function - extend T::Sig - extend T::Helpers - requires_ancestor { Kernel } GlobsToOwningTeamMap = T.type_alias { T::Hash[String, CodeTeams::Team] } # Returns the version of the code_ownership gem and the codeowners-rs gem. sig { returns(T::Array[String]) } - def version + def self.version ["code_ownership version: #{VERSION}", "codeowners-rs version: #{::RustCodeOwners.version}"] end @@ -65,7 +60,7 @@ def version # # => raises exception if no owner found # sig { params(file: String, from_codeowners: T::Boolean, allow_raise: T::Boolean).returns(T.nilable(CodeTeams::Team)) } - def for_file(file, from_codeowners: true, allow_raise: false) + def self.for_file(file, from_codeowners: true, allow_raise: false) if from_codeowners teams_for_files_from_codeowners([file], allow_raise: allow_raise).values.first else @@ -116,7 +111,7 @@ def for_file(file, from_codeowners: true, allow_raise: false) # @see #validate! for ensuring CODEOWNERS file is up-to-date # sig { params(files: T::Array[String], allow_raise: T::Boolean).returns(T::Hash[String, T.nilable(CodeTeams::Team)]) } - def teams_for_files_from_codeowners(files, allow_raise: false) + def self.teams_for_files_from_codeowners(files, allow_raise: false) Private::TeamFinder.teams_for_files(files, allow_raise: allow_raise) end @@ -160,12 +155,12 @@ def teams_for_files_from_codeowners(files, allow_raise: false) # @see CLI#for_file for the command-line interface that uses this method # sig { params(file: String).returns(T.nilable(T::Hash[Symbol, String])) } - def for_file_verbose(file) + def self.for_file_verbose(file) ::RustCodeOwners.for_file(file) end sig { params(team: T.any(CodeTeams::Team, String)).returns(T::Array[String]) } - def for_team(team) + def self.for_team(team) team = T.must(CodeTeams.find(team)) if team.is_a?(String) ::RustCodeOwners.for_team(team.name) end @@ -226,7 +221,7 @@ def for_team(team) files: T.nilable(T::Array[String]) ).void end - def validate!( + def self.validate!( autocorrect: true, stage_changes: true, files: nil @@ -269,7 +264,7 @@ def validate!( # @note Leading newlines after the annotation are also removed to maintain clean formatting. # sig { params(filename: String).void } - def remove_file_annotation!(filename) + def self.remove_file_annotation!(filename) filepath = Pathname.new(filename) begin @@ -292,24 +287,24 @@ def remove_file_annotation!(filename) # Given a backtrace from either `Exception#backtrace` or `caller`, find the # first line that corresponds to a file with assigned ownership sig { params(backtrace: T.nilable(T::Array[String]), excluded_teams: T::Array[::CodeTeams::Team]).returns(T.nilable(::CodeTeams::Team)) } - def for_backtrace(backtrace, excluded_teams: []) + def self.for_backtrace(backtrace, excluded_teams: []) Private::TeamFinder.for_backtrace(backtrace, excluded_teams: excluded_teams) end # Given a backtrace from either `Exception#backtrace` or `caller`, find the # first owned file in it, useful for figuring out which file is being blamed. sig { params(backtrace: T.nilable(T::Array[String]), excluded_teams: T::Array[::CodeTeams::Team]).returns(T.nilable([::CodeTeams::Team, String])) } - def first_owned_file_for_backtrace(backtrace, excluded_teams: []) + def self.first_owned_file_for_backtrace(backtrace, excluded_teams: []) Private::TeamFinder.first_owned_file_for_backtrace(backtrace, excluded_teams: excluded_teams) end - sig { params(klass: T.nilable(T.any(T::Class[T.anything], Module))).returns(T.nilable(::CodeTeams::Team)) } - def for_class(klass) + sig { params(klass: T.nilable(T.any(T::Class[T.anything], T::Module[T.anything]))).returns(T.nilable(::CodeTeams::Team)) } + def self.for_class(klass) Private::TeamFinder.for_class(klass) end sig { params(package: Packs::Pack).returns(T.nilable(::CodeTeams::Team)) } - def for_package(package) + def self.for_package(package) Private::TeamFinder.for_package(package) end diff --git a/lib/code_ownership/cli.rb b/lib/code_ownership/cli.rb index 605682c..79c064d 100644 --- a/lib/code_ownership/cli.rb +++ b/lib/code_ownership/cli.rb @@ -1,4 +1,5 @@ # typed: true +# frozen_string_literal: true require 'optparse' require 'pathname' diff --git a/lib/code_ownership/private/file_path_finder.rb b/lib/code_ownership/private/file_path_finder.rb index e46a32d..2e79881 100644 --- a/lib/code_ownership/private/file_path_finder.rb +++ b/lib/code_ownership/private/file_path_finder.rb @@ -1,19 +1,15 @@ # frozen_string_literal: true - # typed: strict module CodeOwnership module Private module FilePathFinder - module_function - extend T::Sig - extend T::Helpers # Returns a string version of the relative path to a Rails constant, # or nil if it can't find anything - sig { params(klass: T.nilable(T.any(T::Class[T.anything], Module))).returns(T.nilable(String)) } - def path_from_klass(klass) + sig { params(klass: T.nilable(T.any(T::Class[T.anything], T::Module[T.anything]))).returns(T.nilable(String)) } + def self.path_from_klass(klass) if klass path = Object.const_source_location(klass.to_s)&.first (path && Pathname.new(path).relative_path_from(Pathname.pwd).to_s) || nil @@ -23,7 +19,7 @@ def path_from_klass(klass) end sig { params(backtrace: T.nilable(T::Array[String])).returns(T::Enumerable[String]) } - def from_backtrace(backtrace) + def self.from_backtrace(backtrace) return [] unless backtrace # The pattern for a backtrace hasn't changed in forever and is considered diff --git a/lib/code_ownership/private/file_path_team_cache.rb b/lib/code_ownership/private/file_path_team_cache.rb index 5247c39..dfe207d 100644 --- a/lib/code_ownership/private/file_path_team_cache.rb +++ b/lib/code_ownership/private/file_path_team_cache.rb @@ -1,37 +1,33 @@ # frozen_string_literal: true - # typed: strict module CodeOwnership module Private module FilePathTeamCache - module_function - extend T::Sig - extend T::Helpers sig { params(file_path: String).returns(T.nilable(CodeTeams::Team)) } - def get(file_path) + def self.get(file_path) cache[file_path] end sig { params(file_path: String, team: T.nilable(CodeTeams::Team)).void } - def set(file_path, team) + def self.set(file_path, team) cache[file_path] = team end sig { params(file_path: String).returns(T::Boolean) } - def cached?(file_path) + def self.cached?(file_path) cache.key?(file_path) end sig { void } - def bust_cache! + def self.bust_cache! @cache = nil end sig { returns(T::Hash[String, T.nilable(CodeTeams::Team)]) } - def cache + def self.cache @cache ||= T.let(@cache, T.nilable(T::Hash[String, T.nilable(CodeTeams::Team)])) @cache ||= {} diff --git a/lib/code_ownership/private/for_file_output_builder.rb b/lib/code_ownership/private/for_file_output_builder.rb index ab7fac7..a8edb63 100644 --- a/lib/code_ownership/private/for_file_output_builder.rb +++ b/lib/code_ownership/private/for_file_output_builder.rb @@ -1,5 +1,4 @@ # frozen_string_literal: true - # typed: strict module CodeOwnership diff --git a/lib/code_ownership/private/team_finder.rb b/lib/code_ownership/private/team_finder.rb index fc3cb97..76eab7d 100644 --- a/lib/code_ownership/private/team_finder.rb +++ b/lib/code_ownership/private/team_finder.rb @@ -1,19 +1,13 @@ # frozen_string_literal: true - # typed: strict module CodeOwnership module Private module TeamFinder - module_function - extend T::Sig - extend T::Helpers - - requires_ancestor { Kernel } sig { params(file_path: String, allow_raise: T::Boolean).returns(T.nilable(CodeTeams::Team)) } - def for_file(file_path, allow_raise: false) + def self.for_file(file_path, allow_raise: false) return nil if file_path.start_with?('./') return FilePathTeamCache.get(file_path) if FilePathTeamCache.cached?(file_path) @@ -31,7 +25,7 @@ def for_file(file_path, allow_raise: false) end sig { params(files: T::Array[String], allow_raise: T::Boolean).returns(T::Hash[String, T.nilable(CodeTeams::Team)]) } - def teams_for_files(files, allow_raise: false) + def self.teams_for_files(files, allow_raise: false) result = {} # Collect cached results and identify non-cached files @@ -57,8 +51,8 @@ def teams_for_files(files, allow_raise: false) result end - sig { params(klass: T.nilable(T.any(T::Class[T.anything], Module))).returns(T.nilable(::CodeTeams::Team)) } - def for_class(klass) + sig { params(klass: T.nilable(T.any(T::Class[T.anything], T::Module[T.anything]))).returns(T.nilable(::CodeTeams::Team)) } + def self.for_class(klass) file_path = FilePathFinder.path_from_klass(klass) return nil if file_path.nil? @@ -66,7 +60,7 @@ def for_class(klass) end sig { params(package: Packs::Pack).returns(T.nilable(::CodeTeams::Team)) } - def for_package(package) + def self.for_package(package) owner_name = package.raw_hash['owner'] || package.metadata['owner'] return nil if owner_name.nil? @@ -74,12 +68,12 @@ def for_package(package) end sig { params(backtrace: T.nilable(T::Array[String]), excluded_teams: T::Array[::CodeTeams::Team]).returns(T.nilable(::CodeTeams::Team)) } - def for_backtrace(backtrace, excluded_teams: []) + def self.for_backtrace(backtrace, excluded_teams: []) first_owned_file_for_backtrace(backtrace, excluded_teams: excluded_teams)&.first end sig { params(backtrace: T.nilable(T::Array[String]), excluded_teams: T::Array[::CodeTeams::Team]).returns(T.nilable([::CodeTeams::Team, String])) } - def first_owned_file_for_backtrace(backtrace, excluded_teams: []) + def self.first_owned_file_for_backtrace(backtrace, excluded_teams: []) FilePathFinder.from_backtrace(backtrace).each do |file| team = for_file(file) if team && !excluded_teams.include?(team) @@ -91,7 +85,7 @@ def first_owned_file_for_backtrace(backtrace, excluded_teams: []) end sig { params(team_name: String, allow_raise: T::Boolean).returns(T.nilable(CodeTeams::Team)) } - def find_team!(team_name, allow_raise: false) + def self.find_team!(team_name, allow_raise: false) team = CodeTeams.find(team_name) if team.nil? && allow_raise raise(StandardError, "Could not find team with name: `#{team_name}`. Make sure the team is one of `#{CodeTeams.all.map(&:name).sort}`") diff --git a/lib/code_ownership/version.rb b/lib/code_ownership/version.rb index edd7d2c..1b99d6e 100644 --- a/lib/code_ownership/version.rb +++ b/lib/code_ownership/version.rb @@ -1,4 +1,4 @@ -# typed: false +# typed: strict # frozen_string_literal: true module CodeOwnership diff --git a/sorbet/config b/sorbet/config index d1aee61..26caf17 100644 --- a/sorbet/config +++ b/sorbet/config @@ -1,5 +1,3 @@ ---dir -. +--dir=. --ignore=/spec --ignore=/vendor/bundle ---enable-experimental-requires-ancestor diff --git a/spec/lib/code_ownership/private/file_path_team_cache_spec.rb b/spec/lib/code_ownership/private/file_path_team_cache_spec.rb index 9527fd8..8775dda 100644 --- a/spec/lib/code_ownership/private/file_path_team_cache_spec.rb +++ b/spec/lib/code_ownership/private/file_path_team_cache_spec.rb @@ -4,6 +4,10 @@ let(:file_path) { 'app/javascript/[test]/test.js' } let(:codes_team) { instance_double(CodeTeams::Team) } + before do + allow(codes_team).to receive(:is_a?).with(CodeTeams::Team).and_return(true) + end + describe '.get' do subject { described_class.get(file_path) }