Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .reek.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<html>` 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][])

Expand Down Expand Up @@ -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
26 changes: 10 additions & 16 deletions lib/rubycritic/core/analysed_module.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down
18 changes: 7 additions & 11 deletions lib/rubycritic/core/smell.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down
3 changes: 1 addition & 2 deletions rubycritic.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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'
Expand All @@ -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'
Expand Down
126 changes: 126 additions & 0 deletions test/lib/rubycritic/support/type_check_test.rb
Original file line number Diff line number Diff line change
@@ -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
97 changes: 97 additions & 0 deletions test/support/type_check.rb
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
require 'mocha/minitest'
require 'ostruct'
require 'diff/lcs'
require 'support/type_check'

def context(...)
describe(...)
Expand Down
Loading