diff --git a/.reek.yml b/.reek.yml index ad749c6..5d7fa4e 100644 --- a/.reek.yml +++ b/.reek.yml @@ -23,6 +23,8 @@ detectors: - initialize - Skunk::Cli::Application#execute - Skunk::Cli::Options::Argv#parse + - Skunk::Analysis#test_module? + - add_mock_methods_to_module TooManyInstanceVariables: exclude: - Skunk::Generator::Html::FileData @@ -38,6 +40,8 @@ detectors: - Skunk::Command::StatusSharer#share_url_empty? - Skunk::Configuration#supported_format? - Skunk::Configuration#supported_formats + - Skunk::Analysis#test_module? + - Skunk::ConfigTest#setup ManualDispatch: exclude: - Skunk::Config#self.method_missing @@ -45,3 +49,11 @@ detectors: BooleanParameter: exclude: - Skunk::Config#self.respond_to_missing? + DuplicateMethodCall: + exclude: + - Skunk::ConfigTest#test_add_format_ignores_duplicates + FeatureEnvy: + exclude: + - Skunk::Command::StatusReporter#table + - Skunk::Generator::HtmlReport#create_directories_and_files + - add_mock_methods_to_module diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index e101f19..cc7705f 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2025-10-17 11:20:00 UTC using RuboCop version 1.81.1. +# on 2025-10-17 16:20:40 UTC using RuboCop version 1.81.1. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -36,11 +36,11 @@ Lint/MissingSuper: Metrics/AbcSize: Max: 18 -# Offense count: 8 +# Offense count: 12 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns. # AllowedMethods: refine Metrics/BlockLength: - Max: 82 + Max: 233 # Offense count: 2 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns. diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cbe885..e6bc057 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## main [(unreleased)](https://github.com/fastruby/skunk/compare/v0.5.4...HEAD) +* [REFACTOR: Centralize Skunk analysis into RubyCritic module](https://github.com/fastruby/skunk/pull/127) * [FEATURE: Add Skunk HTML Report](https://github.com/fastruby/skunk/pull/123) * [FEATURE: Add Skunk::Config class](https://github.com/fastruby/skunk/pull/123) * [FEATURE: Add Ruby 3.4 compatibility](https://github.com/fastruby/skunk/pull/124) diff --git a/lib/skunk/commands/status_reporter.rb b/lib/skunk/commands/status_reporter.rb index be43eef..315347a 100644 --- a/lib/skunk/commands/status_reporter.rb +++ b/lib/skunk/commands/status_reporter.rb @@ -10,6 +10,10 @@ module Command class StatusReporter < RubyCritic::Command::StatusReporter attr_accessor :analysed_modules + def initialize(options = {}) + super(options) + end + HEADINGS = %w[file skunk_score churn_times_cost churn cost coverage].freeze HEADINGS_WITHOUT_FILE = HEADINGS - %w[file] HEADINGS_WITHOUT_FILE_WIDTH = HEADINGS_WITHOUT_FILE.size * 17 # padding @@ -38,36 +42,27 @@ def update_status_message private def analysed_modules_count - @analysed_modules_count ||= non_test_modules.count - end - - def non_test_modules - @non_test_modules ||= analysed_modules.reject do |a_module| - module_path = a_module.pathname.dirname.to_s - module_path.start_with?("test", "spec") || module_path.end_with?("test", "spec") - end + analysed_modules.analysed_modules_count end def worst - @worst ||= sorted_modules.first + analysed_modules.worst_module end def sorted_modules - @sorted_modules ||= non_test_modules.sort_by(&:skunk_score).reverse! + analysed_modules.sorted_modules end def total_skunk_score - @total_skunk_score ||= non_test_modules.sum(&:skunk_score) + analysed_modules.skunk_score_total end def total_churn_times_cost - non_test_modules.sum(&:churn_times_cost) + analysed_modules.total_churn_times_cost end def skunk_score_average - return 0 if analysed_modules_count.zero? - - (total_skunk_score.to_d / analysed_modules_count).to_f.round(2) + analysed_modules.skunk_score_average end def table_options diff --git a/lib/skunk/generators/html/overview.rb b/lib/skunk/generators/html/overview.rb index 1accdfe..78e7d2b 100644 --- a/lib/skunk/generators/html/overview.rb +++ b/lib/skunk/generators/html/overview.rb @@ -3,7 +3,8 @@ require "rubycritic/generators/html/base" require "skunk/generators/html/path_truncator" -require "skunk/generators/html/skunk_data" +require "skunk/generators/html/file_data" +require "skunk/rubycritic/analysed_modules_collection" module Skunk module Generator @@ -19,7 +20,8 @@ def self.erb_template(template_path) def initialize(analysed_modules) @analysed_modules = analysed_modules - @data = SkunkData.new(analysed_modules) + @generated_at = Time.now.strftime("%Y-%m-%d %H:%M:%S") + @skunk_version = Skunk::VERSION end def file_name @@ -29,6 +31,24 @@ def file_name def render TEMPLATE.result(binding) end + + def analysed_modules_count + @analysed_modules.analysed_modules_count + end + + def skunk_score_total + @analysed_modules.skunk_score_total + end + + def skunk_score_average + @analysed_modules.skunk_score_average + end + + def files + @files ||= @analysed_modules.sorted_modules.map do |module_data| + FileData.new(module_data) + end + end end end end diff --git a/lib/skunk/generators/html/skunk_data.rb b/lib/skunk/generators/html/skunk_data.rb deleted file mode 100644 index 19eadb0..0000000 --- a/lib/skunk/generators/html/skunk_data.rb +++ /dev/null @@ -1,59 +0,0 @@ -# frozen_string_literal: true - -require "skunk/generators/html/file_data" -require "skunk/generators/html/path_truncator" - -module Skunk - module Generator - module Html - # Data object for the HTML overview report - class SkunkData - attr_reader :generated_at, :skunk_version, - :analysed_modules_count, :skunk_score_total, :skunk_score_average, - :worst_pathname, :worst_score, :files - - def initialize(analysed_modules) - @analysed_modules = analysed_modules - @generated_at = Time.now.strftime("%Y-%m-%d %H:%M:%S") - @skunk_version = Skunk::VERSION - - @analysed_modules_count = non_test_modules.count - @skunk_score_total = non_test_modules.sum(&:skunk_score) - @skunk_score_average = calculate_average - @worst_pathname = PathTruncator.truncate(find_worst_module&.pathname) - @worst_score = find_worst_module&.skunk_score - @files = build_files - end - - private - - def non_test_modules - @non_test_modules ||= @analysed_modules.reject do |a_module| - module_path = a_module.pathname.dirname.to_s - module_path.start_with?("test", "spec") || module_path.end_with?("test", "spec") - end - end - - def calculate_average - return 0 if @analysed_modules_count.zero? - - (@skunk_score_total.to_d / @analysed_modules_count).round(2) - end - - def find_worst_module - @find_worst_module ||= sorted_modules.first - end - - def sorted_modules - @sorted_modules ||= non_test_modules.sort_by(&:skunk_score).reverse! - end - - def build_files - @build_files ||= sorted_modules.map do |module_data| - FileData.new(module_data) - end - end - end - end - end -end diff --git a/lib/skunk/generators/html/templates/skunk_overview.html.erb b/lib/skunk/generators/html/templates/skunk_overview.html.erb index 9fea17e..cee421a 100644 --- a/lib/skunk/generators/html/templates/skunk_overview.html.erb +++ b/lib/skunk/generators/html/templates/skunk_overview.html.erb @@ -242,15 +242,15 @@
-
<%= @data.analysed_modules_count %>
+
<%= analysed_modules_count %>
Modules Analysed
-
<%= @data.skunk_score_total %>
+
<%= skunk_score_total %>
Total Skunk Score
-
<%= @data.skunk_score_average %>
+
<%= skunk_score_average %>
Average Skunk Score
@@ -269,7 +269,7 @@ - <% @data.files.each do |item| %> + <% files.each do |item| %> <%= item.file %> @@ -301,7 +301,7 @@ diff --git a/lib/skunk/generators/json/simple.rb b/lib/skunk/generators/json/simple.rb index 1dc2086..0a686ce 100644 --- a/lib/skunk/generators/json/simple.rb +++ b/lib/skunk/generators/json/simple.rb @@ -3,6 +3,7 @@ require "pathname" require "rubycritic/configuration" +require "skunk/rubycritic/analysed_modules_collection" module Skunk module Generator @@ -20,14 +21,7 @@ def render end def data - { - analysed_modules_count: analysed_modules_count, - skunk_score_total: skunk_score_total, - skunk_score_average: calculate_average, - worst_pathname: find_worst_module&.pathname, - worst_score: find_worst_module&.skunk_score, - files: build_files - } + @analysed_modules.to_hash end def file_directory @@ -37,41 +31,6 @@ def file_directory def file_pathname Pathname.new(file_directory).join(FILE_NAME) end - - private - - def analysed_modules_count - @analysed_modules_count ||= non_test_modules.count - end - - def calculate_average - return 0 if analysed_modules_count.zero? - - (skunk_score_total.to_d / analysed_modules_count).to_f.round(2) - end - - def skunk_score_total - @skunk_score_total ||= non_test_modules.sum(&:skunk_score) - end - - def non_test_modules - @non_test_modules ||= @analysed_modules.reject do |a_module| - module_path = a_module.pathname.dirname.to_s - module_path.start_with?("test", "spec") || module_path.end_with?("test", "spec") - end - end - - def find_worst_module - @find_worst_module ||= sorted_modules.first - end - - def sorted_modules - @sorted_modules ||= non_test_modules.sort_by(&:skunk_score).reverse! - end - - def build_files - @build_files ||= sorted_modules.map(&:to_hash) - end end end end diff --git a/lib/skunk/rubycritic/analysed_modules_collection.rb b/lib/skunk/rubycritic/analysed_modules_collection.rb index 297aaf0..d03fc05 100644 --- a/lib/skunk/rubycritic/analysed_modules_collection.rb +++ b/lib/skunk/rubycritic/analysed_modules_collection.rb @@ -3,15 +3,100 @@ require "rubycritic/core/analysed_modules_collection" module RubyCritic - # nodoc # + # Extends RubyCritic::AnalysedModulesCollection to add Skunk analysis methods class AnalysedModulesCollection + # Returns the count of non-test modules + # @return [Integer] + def analysed_modules_count + @analysed_modules_count ||= non_test_modules.count + end + + # Returns the total Skunk score across all non-test modules + # @return [Float] + def skunk_score_total + @skunk_score_total ||= non_test_modules.sum(&:skunk_score) + end + + # Returns the average Skunk score across all non-test modules + # @return [Float] def skunk_score_average - num_modules = @modules.size - if num_modules.positive? - sum(&:skunk_score) / num_modules.to_f - else - 0.0 + return 0.0 if analysed_modules_count.zero? + + (skunk_score_total.to_d / analysed_modules_count).to_f.round(2) + end + + # Returns the total churn times cost across all non-test modules + # @return [Float] + def total_churn_times_cost + @total_churn_times_cost ||= non_test_modules.sum(&:churn_times_cost) + end + + # Returns the module with the highest Skunk score (worst performing) + # @return [RubyCritic::AnalysedModule, nil] + def worst_module + @worst_module ||= sorted_modules.first + end + + # Returns modules sorted by Skunk score in descending order (worst first) + # @return [Array] + def sorted_modules + @sorted_modules ||= non_test_modules.sort_by(&:skunk_score).reverse! + end + + # Returns only non-test modules (excludes test and spec directories) + # @return [Array] + def non_test_modules + @non_test_modules ||= reject do |a_module| + test_module?(a_module) end end + + # Returns a hash representation of the analysis results + # @return [Hash] + def to_hash + { + analysed_modules_count: analysed_modules_count, + skunk_score_total: skunk_score_total, + skunk_score_average: skunk_score_average, + total_churn_times_cost: total_churn_times_cost, + worst_pathname: worst_module&.pathname, + worst_score: worst_module&.skunk_score, + files: files_as_hash + } + end + + private + + # Returns files as an array of hashes (for JSON serialization) + # @return [Array] + def files_as_hash + @files_as_hash ||= sorted_modules.map(&:to_hash) + end + + # Determines if a module is a test module based on its path + # @param a_module [RubyCritic::AnalysedModule] The module to check + # @return [Boolean] + def test_module?(a_module) + pathname = a_module.pathname + directory_is_test?(pathname) || filename_is_test?(pathname) + end + + # Checks if the directory path indicates a test module + # @param pathname [Pathname] The pathname to check + # @return [Boolean] + # :reek:UtilityFunction + def directory_is_test?(pathname) + module_path = pathname.dirname.to_s + module_path.start_with?("test", "spec") || module_path.end_with?("test", "spec") + end + + # Checks if the filename indicates a test module + # @param pathname [Pathname] The pathname to check + # @return [Boolean] + # :reek:UtilityFunction + def filename_is_test?(pathname) + filename = pathname.basename.to_s + filename.end_with?("_test.rb", "_spec.rb") + end end end diff --git a/test/lib/skunk/commands/status_reporter_test.rb b/test/lib/skunk/commands/status_reporter_test.rb index 92302d9..26585ab 100644 --- a/test/lib/skunk/commands/status_reporter_test.rb +++ b/test/lib/skunk/commands/status_reporter_test.rb @@ -3,6 +3,7 @@ require "test_helper" require "rubycritic/analysers_runner" +require "skunk/rubycritic/analysed_modules_collection" require "skunk/commands/status_reporter" describe Skunk::Command::StatusReporter do diff --git a/test/lib/skunk/config_test.rb b/test/lib/skunk/config_test.rb index ceaf304..3664d63 100644 --- a/test/lib/skunk/config_test.rb +++ b/test/lib/skunk/config_test.rb @@ -4,7 +4,9 @@ require_relative "../../../lib/skunk/config" module Skunk + # Test class for Skunk::Config functionality class ConfigTest < Minitest::Test + # Reset configuration before each test to ensure clean state def setup Config.reset end @@ -40,7 +42,7 @@ def test_add_format def test_add_format_ignores_duplicates Config.add_format(:html) - Config.add_format(:html) + Config.add_format(:html) # This should be ignored as duplicate assert_equal %i[json html], Config.formats end diff --git a/test/lib/skunk/generators/html/path_truncator_test.rb b/test/lib/skunk/generators/html/path_truncator_test.rb index e8236ee..695ab4e 100644 --- a/test/lib/skunk/generators/html/path_truncator_test.rb +++ b/test/lib/skunk/generators/html/path_truncator_test.rb @@ -4,7 +4,6 @@ require "skunk/generators/html/path_truncator" -# rubocop:disable Metrics/BlockLength describe Skunk::Generator::Html::PathTruncator do describe ".truncate" do context "when path contains app folder" do @@ -176,4 +175,3 @@ end end end -# rubocop:enable Metrics/BlockLength diff --git a/test/lib/skunk/rubycritic/analysed_modules_collection_test.rb b/test/lib/skunk/rubycritic/analysed_modules_collection_test.rb new file mode 100644 index 0000000..5bf4e92 --- /dev/null +++ b/test/lib/skunk/rubycritic/analysed_modules_collection_test.rb @@ -0,0 +1,285 @@ +# frozen_string_literal: true + +require "test_helper" + +require "skunk/rubycritic/analysed_modules_collection" +require "skunk/rubycritic/analysed_module" + +describe RubyCritic::AnalysedModulesCollection do + let(:analysed_modules) { [] } + let(:collection) { create_collection(analysed_modules) } + + describe "#analysed_modules_count" do + context "with no modules" do + it "returns 0" do + _(collection.analysed_modules_count).must_equal 0 + end + end + + context "with non-test modules" do + let(:analysed_modules) { [create_analysed_module("lib/file.rb"), create_analysed_module("app/model.rb")] } + + it "returns the count of non-test modules" do + _(collection.analysed_modules_count).must_equal 2 + end + end + + context "with test modules" do + let(:analysed_modules) do + [ + create_analysed_module("lib/file.rb"), + create_analysed_module("test/file_test.rb"), + create_analysed_module("spec/file_spec.rb") + ] + end + + it "excludes test modules from count" do + _(collection.analysed_modules_count).must_equal 1 + end + end + end + + describe "#skunk_score_total" do + context "with no modules" do + it "returns 0" do + _(collection.skunk_score_total).must_equal 0 + end + end + + context "with modules" do + let(:analysed_modules) do + [ + create_analysed_module("lib/file1.rb", skunk_score: 10.5), + create_analysed_module("lib/file2.rb", skunk_score: 20.3) + ] + end + + it "returns the sum of skunk scores" do + _(collection.skunk_score_total).must_equal 30.8 + end + end + + context "with test modules" do + let(:analysed_modules) do + [ + create_analysed_module("lib/file.rb", skunk_score: 10.0), + create_analysed_module("test/file_test.rb", skunk_score: 50.0) + ] + end + + it "excludes test modules from total" do + _(collection.skunk_score_total).must_equal 10.0 + end + end + end + + describe "#skunk_score_average" do + context "with no modules" do + it "returns 0" do + _(collection.skunk_score_average).must_equal 0.0 + end + end + + context "with modules" do + let(:analysed_modules) do + [ + create_analysed_module("lib/file1.rb", skunk_score: 10.0), + create_analysed_module("lib/file2.rb", skunk_score: 20.0) + ] + end + + it "returns the average skunk score" do + _(collection.skunk_score_average).must_equal 15.0 + end + end + + context "with decimal average" do + let(:analysed_modules) do + [ + create_analysed_module("lib/file1.rb", skunk_score: 10.0), + create_analysed_module("lib/file2.rb", skunk_score: 11.0) + ] + end + + it "rounds to 2 decimal places" do + _(collection.skunk_score_average).must_equal 10.5 + end + end + end + + describe "#total_churn_times_cost" do + context "with no modules" do + it "returns 0" do + _(collection.total_churn_times_cost).must_equal 0 + end + end + + context "with modules" do + let(:analysed_modules) do + [ + create_analysed_module("lib/file1.rb", churn_times_cost: 5.0), + create_analysed_module("lib/file2.rb", churn_times_cost: 15.0) + ] + end + + it "returns the sum of churn times cost" do + _(collection.total_churn_times_cost).must_equal 20.0 + end + end + end + + describe "#worst_module" do + context "with no modules" do + it "returns nil" do + _(collection.worst_module).must_be_nil + end + end + + context "with modules" do + let(:worst_module) { create_analysed_module("lib/worst.rb", skunk_score: 100.0) } + let(:best_module) { create_analysed_module("lib/best.rb", skunk_score: 10.0) } + let(:analysed_modules) { [best_module, worst_module] } + + it "returns the module with highest skunk score" do + _(collection.worst_module).must_equal worst_module + end + end + end + + describe "#sorted_modules" do + context "with no modules" do + it "returns empty array" do + _(collection.sorted_modules).must_equal [] + end + end + + context "with modules" do + let(:module1) { create_analysed_module("lib/file1.rb", skunk_score: 10.0) } + let(:module2) { create_analysed_module("lib/file2.rb", skunk_score: 30.0) } + let(:module3) { create_analysed_module("lib/file3.rb", skunk_score: 20.0) } + let(:analysed_modules) { [module1, module2, module3] } + + it "returns modules sorted by skunk score descending" do + _(collection.sorted_modules).must_equal [module2, module3, module1] + end + end + + context "with test modules" do + let(:spec_module) { create_analysed_module("test/file_test.rb", skunk_score: 100.0) } + let(:lib_module) { create_analysed_module("lib/file.rb", skunk_score: 10.0) } + let(:analysed_modules) { [spec_module, lib_module] } + + it "excludes test modules from sorted list" do + _(collection.sorted_modules).must_equal [lib_module] + end + end + end + + describe "#non_test_modules" do + context "with mixed modules" do + let(:analysed_modules) do + [ + create_analysed_module("lib/file.rb"), + create_analysed_module("test/file_test.rb"), + create_analysed_module("spec/file_spec.rb"), + create_analysed_module("app/model.rb") + ] + end + + it "filters out test and spec modules" do + non_test = collection.non_test_modules + _(non_test.size).must_equal 2 + _(non_test.map(&:pathname).map(&:to_s)).must_include "lib/file.rb" + _(non_test.map(&:pathname).map(&:to_s)).must_include "app/model.rb" + end + end + + context "with modules in test directories" do + let(:analysed_modules) do + [ + create_analysed_module("test/unit/file.rb"), + create_analysed_module("spec/unit/file.rb"), + create_analysed_module("lib/file.rb") + ] + end + + it "filters out modules in test directories" do + non_test = collection.non_test_modules + _(non_test.size).must_equal 1 + _(non_test.first.pathname.to_s).must_equal "lib/file.rb" + end + end + + context "with modules ending in test/spec" do + let(:analysed_modules) do + [ + create_analysed_module("lib/file_test.rb"), + create_analysed_module("lib/file_spec.rb"), + create_analysed_module("lib/file.rb") + ] + end + + it "filters out modules ending in test/spec" do + non_test = collection.non_test_modules + _(non_test.size).must_equal 1 + _(non_test.first.pathname.to_s).must_equal "lib/file.rb" + end + end + end + + describe "#to_hash" do + let(:analysed_modules) do + [ + create_analysed_module("lib/file.rb", skunk_score: 10.0, churn_times_cost: 5.0) + ] + end + + it "returns a hash with all analysis data including files" do + hash = collection.to_hash + _(hash[:analysed_modules_count]).must_equal 1 + _(hash[:skunk_score_total]).must_equal 10.0 + _(hash[:skunk_score_average]).must_equal 10.0 + _(hash[:total_churn_times_cost]).must_equal 5.0 + _(hash[:worst_pathname]).must_equal Pathname.new("lib/file.rb") + _(hash[:worst_score]).must_equal 10.0 + _(hash[:files]).must_be_kind_of Array + _(hash[:files].size).must_equal 1 + _(hash[:files].first[:file]).must_equal "lib/file.rb" + _(hash[:files].first[:skunk_score]).must_equal 10.0 + end + end + + private + + def create_collection(modules) + # Create a collection by manually setting the @modules instance variable + # This bypasses the complex initialization that expects file paths + collection = RubyCritic::AnalysedModulesCollection.new([], []) + collection.instance_variable_set(:@modules, modules) + collection + end + + def create_analysed_module(path, skunk_score: 0.0, churn_times_cost: 0.0) + module_path = Pathname.new(path) + analysed_module = RubyCritic::AnalysedModule.new( + pathname: module_path, + smells: [], + churn: 1, + committed_at: Time.now + ) + + add_mock_methods_to_module(analysed_module, skunk_score, churn_times_cost) + end + + def add_mock_methods_to_module(analysed_module, skunk_score, churn_times_cost) + # Mock the skunk_score and churn_times_cost methods + analysed_module.define_singleton_method(:skunk_score) { @skunk_score ||= 0.0 } + analysed_module.define_singleton_method(:skunk_score=) { |value| @skunk_score = value } + analysed_module.define_singleton_method(:churn_times_cost) { @churn_times_cost ||= 0.0 } + analysed_module.define_singleton_method(:churn_times_cost=) { |value| @churn_times_cost = value } + + analysed_module.skunk_score = skunk_score + analysed_module.churn_times_cost = churn_times_cost + analysed_module + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 1b63517..3d4c2f1 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -27,6 +27,7 @@ require "minitest/stub_any_instance" require "webmock/minitest" +require "skunk" require "skunk/rubycritic/analysed_module" def context(*args, &block)