diff --git a/.reek.yml b/.reek.yml index 8fa5f167..634f93c0 100644 --- a/.reek.yml +++ b/.reek.yml @@ -92,6 +92,23 @@ detectors: - RubyCritic::RakeTask#paths - RubyCritic::RakeTask#verbose - RubyCritic::RakeTask#fail_on_error + - RubyCritic::Smell#context + - RubyCritic::Smell#cost + - RubyCritic::Smell#locations + - RubyCritic::Smell#message + - RubyCritic::Smell#score + - RubyCritic::Smell#status + - RubyCritic::Smell#type + - RubyCritic::Smell#analyser + - RubyCritic::AnalysedModule#coverage + - RubyCritic::AnalysedModule#name + - RubyCritic::AnalysedModule#pathname + - RubyCritic::AnalysedModule#smells + - RubyCritic::AnalysedModule#churn + - RubyCritic::AnalysedModule#committed_at + - RubyCritic::AnalysedModule#complexity + - RubyCritic::AnalysedModule#duplication + - RubyCritic::AnalysedModule#methods_count DuplicateMethodCall: exclude: - RubyCritic::Analyser::Churn#run @@ -113,6 +130,7 @@ detectors: - RubyCritic::Reporter#self.report_generator_class - RubyCritic::SourceLocator#deduplicate_symlinks - RubyCritic::Command::Compare#compare_branches + - RubyCritic::AnalysedModule#initialize FeatureEnvy: exclude: - Parser::AST::Node#module_name @@ -165,6 +183,7 @@ detectors: - RubyCritic::SourceLocator#ruby_file? TooManyInstanceVariables: exclude: + - RubyCritic::AnalysedModule - RubyCritic::Generator::Html::CodeFile - RubyCritic::Generator::Html::Overview - RubyCritic::Generator::Html::SmellsIndex diff --git a/.rubocop.yml b/.rubocop.yml index 8d6ffb2b..0133f989 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -54,6 +54,11 @@ Metrics/MethodLength: Exclude: - 'lib/rubycritic/configuration.rb' - 'test/integration/integration_test_helper.rb' + - 'test/support/type_check.rb' + +Naming/MethodName: + Exclude: + - 'test/support/type_check.rb' Naming/RescuedExceptionsVariableName: Exclude: @@ -78,6 +83,7 @@ Style/OneClassPerFile: - 'test/analysers_test_helper.rb' - 'test/fakefs_helper.rb' - 'test/test_helper.rb' + - 'test/support/type_check.rb' Style/OpenStructUse: Exclude: diff --git a/CHANGELOG.md b/CHANGELOG.md index 1316935f..3ad08282 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * [FEATURE] ... * [CHANGE] Replace Aruba with direct API calls in specs (by [@faisal][]) +* [CHANGE] Replace the unmaintained `virtus` dependency with plain Ruby accessors, dropping its abandoned transitive dependencies (`axiom-types`, `coercible`, `descendants_tracker`, `ice_nine`, and the deprecated `thread_safe`). Also move `ostruct` to a development dependency since it is only used in tests. (by [@pnomolos][]) * [CHANGE] Replace all Cucumber features with Minitest/Spec specs (by [@faisal][]) * [BUGFIX] Add `lang="en"` to the report's `` element, give the menu-toggle anchor an `aria-label`, and make the per-rating summary IDs unique. Fixes 17 WCAG 2.1 AA structural errors on `overview.html`. (by [@MarcusAl][]) @@ -507,3 +508,4 @@ [@exoego]: https://github.com/exoego [@raff-s]: https://github.com/raff-s [@MarcusAl]: https://github.com/MarcusAl +[@pnomolos]: https://github.com/pnomolos diff --git a/lib/rubycritic/core/analysed_module.rb b/lib/rubycritic/core/analysed_module.rb index 441a091b..75a6fdbe 100644 --- a/lib/rubycritic/core/analysed_module.rb +++ b/lib/rubycritic/core/analysed_module.rb @@ -1,28 +1,22 @@ # frozen_string_literal: true -require 'virtus' require 'rubycritic/core/rating' module RubyCritic class AnalysedModule - include Virtus.model - # Complexity is reduced by a factor of 25 when calculating cost COMPLEXITY_FACTOR = 25.0 - attribute :coverage, Float, default: 0.0 - attribute :name - attribute :smells_count - attribute :file_location - attribute :file_name - attribute :line_count - attribute :pathname - attribute :smells, Array, default: [] - attribute :churn - attribute :committed_at - attribute :complexity, Float, default: Float::INFINITY - attribute :duplication, Integer, default: 0 - attribute :methods_count + attr_accessor :coverage, :name, :pathname, :smells, :churn, :committed_at, :complexity, :duplication, :methods_count + + def initialize(attributes = {}) + @coverage = 0.0 + @smells = [] + @complexity = Float::INFINITY + @duplication = 0 + @methods_count = 0 + attributes.each { |name, value| public_send("#{name}=", value) } + end def path @path ||= pathname.to_s diff --git a/lib/rubycritic/core/smell.rb b/lib/rubycritic/core/smell.rb index 37dcb9f2..8f31b3af 100644 --- a/lib/rubycritic/core/smell.rb +++ b/lib/rubycritic/core/smell.rb @@ -1,20 +1,16 @@ # frozen_string_literal: true -require 'virtus' require 'rubycritic/core/location' module RubyCritic class Smell - include Virtus.model - - attribute :context - attribute :cost - attribute :locations, Array, default: [] - attribute :message - attribute :score - attribute :status, Symbol, default: :new - attribute :type - attribute :analyser + attr_accessor :context, :cost, :locations, :message, :score, :status, :type, :analyser + + def initialize(attributes = {}) + @locations = [] + @status = :new + attributes.each { |name, value| public_send("#{name}=", value) } + end FLAY_DOCS_URL = 'http://docs.seattlerb.org/flay/'.freeze FLOG_DOCS_URL = 'http://docs.seattlerb.org/flog/'.freeze diff --git a/rubycritic.gemspec b/rubycritic.gemspec index 5763f156..a865e476 100644 --- a/rubycritic.gemspec +++ b/rubycritic.gemspec @@ -34,7 +34,6 @@ Gem::Specification.new do |spec| spec.add_dependency 'flay', '~> 2.13' spec.add_dependency 'flog', '~> 4.7' spec.add_dependency 'launchy', '>= 2.5.2' - spec.add_dependency 'ostruct' spec.add_dependency 'parser', '>= 3.3.0.5' spec.add_dependency 'prism', '>= 1.6.0' spec.add_dependency 'rainbow', '~> 3.1.1' @@ -43,7 +42,6 @@ Gem::Specification.new do |spec| spec.add_dependency 'ruby_parser', '~> 3.21' spec.add_dependency 'simplecov', '>= 0.22.0' spec.add_dependency 'tty-which', '~> 0.5.0' - spec.add_dependency 'virtus', '~> 2.0' spec.add_development_dependency 'bundler', '>= 2.0.0' if RUBY_PLATFORM == 'java' @@ -59,6 +57,7 @@ Gem::Specification.new do |spec| spec.add_development_dependency 'minitest-around', '~> 0.6.0' spec.add_development_dependency 'minitest-mock' spec.add_development_dependency 'mocha', '~> 3.0.0' + spec.add_development_dependency 'ostruct' spec.add_development_dependency 'rake', '~> 13.3.0', '>= 11.0.0' spec.add_development_dependency 'rdoc' spec.add_development_dependency 'rexml', '>= 3.2.0' diff --git a/test/lib/rubycritic/support/type_check_test.rb b/test/lib/rubycritic/support/type_check_test.rb new file mode 100644 index 00000000..722127f3 --- /dev/null +++ b/test/lib/rubycritic/support/type_check_test.rb @@ -0,0 +1,126 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'rubycritic/core/smell' +require 'rubycritic/core/analysed_module' + +describe 'TypeCheck' do + describe RubyCritic::Smell do + it 'allows a value of the declared type' do + smell = RubyCritic::Smell.new + smell.cost = 42 + + _(smell.cost).must_equal 42 + end + + it 'allows a subclass of the declared type (Integer for Numeric)' do + smell = RubyCritic::Smell.new + smell.score = 3 + + _(smell.score).must_equal 3 + end + + it 'allows nil for any attribute' do + smell = RubyCritic::Smell.new + smell.message = nil + + _(smell.message).must_be_nil + end + + it 'accepts either member of a union type (String or Symbol for type)' do + smell = RubyCritic::Smell.new + smell.type = 'DuplicateCode' + + _(smell.type).must_equal 'DuplicateCode' + smell.type = :complexity + + _(smell.type).must_equal :complexity + end + + it 'raises a TypeError when assigning the wrong type' do + smell = RubyCritic::Smell.new + error = _ { smell.cost = 'expensive' }.must_raise TypeError + _(error.message).must_match(/Smell#cost= expected Numeric or nil, got String/) + end + + it 'raises when assigning the wrong type to a union attribute' do + smell = RubyCritic::Smell.new + error = _ { smell.type = 123 }.must_raise TypeError + _(error.message).must_match(/Smell#type= expected String \| Symbol or nil, got Integer/) + end + + it 'enforces the type through the initializer' do + _ { RubyCritic::Smell.new(locations: 'not-an-array') }.must_raise TypeError + end + + it 'constructs with realistic arguments without raising' do + smell = RubyCritic::Smell.new( + context: '#bar', + cost: 16, + locations: [RubyCritic::Location.new('./foo', '42')], + message: 'This smells', + score: 0, + type: 'DuplicateMethodCall', + analyser: 'flog' + ) + + _(smell.analyser).must_equal 'flog' + end + end + + describe RubyCritic::AnalysedModule do + it 'allows an Integer for a Numeric attribute (coverage)' do + mod = RubyCritic::AnalysedModule.new + mod.coverage = 0 + + _(mod.coverage).must_equal 0 + end + + it 'allows a Float for a Numeric attribute (coverage)' do + mod = RubyCritic::AnalysedModule.new + mod.coverage = 87.5 + + _(mod.coverage).must_equal 87.5 + end + + it 'preserves the Float::INFINITY default' do + _(RubyCritic::AnalysedModule.new.complexity).must_equal Float::INFINITY + end + + it 'accumulates duplication through the writer (+=)' do + mod = RubyCritic::AnalysedModule.new + mod.duplication += 18 + + _(mod.duplication).must_equal 18 + end + + it 'raises a TypeError when assigning the wrong type to pathname' do + mod = RubyCritic::AnalysedModule.new + error = _ { mod.pathname = 'foo.rb' }.must_raise TypeError + _(error.message).must_match(/AnalysedModule#pathname= expected Pathname \| FakeFS::Pathname or nil, got String/) + end + + it 'raises when assigning a non-Array to smells' do + _ { RubyCritic::AnalysedModule.new.smells = 5 }.must_raise TypeError + end + + it 'constructs with realistic arguments without raising' do + mod = RubyCritic::AnalysedModule.new( + name: 'Foo', + pathname: Pathname.new('foo.rb'), + smells: [], + churn: 3, + complexity: 12.5, + methods_count: 4 + ) + + _(mod.name).must_equal 'Foo' + end + end + + describe 'the TypeCheck error type' do + it 'is a kind of the standard TypeError' do + _(TypeCheckHelper::TypeError.ancestors).must_include TypeError + end + end +end diff --git a/test/support/type_check.rb b/test/support/type_check.rb new file mode 100644 index 00000000..5365b846 --- /dev/null +++ b/test/support/type_check.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'pathname' +require 'rubycritic/core/smell' +require 'rubycritic/core/analysed_module' + +# Test-only "micro-Sorbet". Wraps attribute writers so that assigning a value +# of an unexpected type raises immediately, catching type regressions in the +# plain-Ruby accessors that replaced virtus. It is loaded only through +# test_helper, is never required by production code, and is never shipped +# (the gemspec packages `lib` only), so it has zero runtime impact. +module TypeCheckHelper + # Raised when a wrapped writer receives a value of an unexpected type. + class TypeError < ::TypeError; end + + # Does `value` satisfy any of the `allowed` type entries? An entry may be a + # Module (matched with is_a?) or a String naming a class that might not be + # loaded when the spec is defined (e.g. the FakeFS::Pathname test double), + # matched by name against the value's ancestors. + def self.match?(value, allowed) + allowed.any? do |type| + if type.is_a?(Module) + value.is_a?(type) + else + value.class.ancestors.any? { |ancestor| ancestor.name == type } + end + end + end + + # Builds an anonymous module that, when included, prepends type-checking + # wrappers around the named writers of the including class. `prepend` is + # required (not plain include) so the wrapper's `name=` wins over the + # `attr_accessor` writer defined directly on the class, while `super` + # still reaches that original writer. + def self.build(specs) + Module.new do + define_singleton_method(:included) do |base| + wrapper = Module.new do + specs.each do |name, expected| + allowed = Array(expected) + define_method("#{name}=") do |value| + unless value.nil? || TypeCheckHelper.match?(value, allowed) + raise TypeCheckHelper::TypeError, + "#{base}##{name}= expected #{allowed.join(' | ')} or nil, " \ + "got #{value.class} (#{value.inspect})" + end + + super(value) + end + end + end + + base.prepend(wrapper) + end + end + end +end + +# Test-only DSL so a class can declare `include TypeCheck(attr: Type, ...)`. +# Defined privately on Kernel so the bare `TypeCheck(...)` call resolves inside +# a class body (where `self` is the class). A nil value is always permitted; a +# spec value may be a single class or an array of classes (a union). +module Kernel + private + + def TypeCheck(specs) + TypeCheckHelper.build(specs) + end +end + +module RubyCritic + class Smell + include TypeCheck( + cost: Numeric, + score: Numeric, + locations: Array, + status: Symbol, + context: String, + message: String, + type: [String, Symbol], + analyser: String + ) + end + + class AnalysedModule + include TypeCheck( + coverage: Numeric, + complexity: Numeric, + duplication: Numeric, + churn: Integer, + methods_count: Integer, + name: String, + pathname: [Pathname, 'FakeFS::Pathname'], + smells: Array + ) + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index c92b23b5..5dac7ab0 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -13,6 +13,7 @@ require 'mocha/minitest' require 'ostruct' require 'diff/lcs' +require 'support/type_check' def context(...) describe(...)