diff --git a/API.md b/API.md index 12c77a3..88317c3 100644 --- a/API.md +++ b/API.md @@ -36,6 +36,13 @@ keys such as `{ in_folder: 'generated', with_name: '*_spec.rb' }`. ## File rules +Every completed rule implementing `ArchUnit::Checkable` supports `.because(reason)`. Append it +after all selectors and modifiers to get a distinct frozen rule of the same type, with a trimmed, +immutable `rationale` string. Non-string or blank reasons raise `ArgumentError`. The original rule, +violations, graph cache, and `check` options are unchanged. The shared result formatter adds +`Because: ...` exactly once to failed framework-neutral, RSpec, and Minitest assertions, including +failed negated RSpec expectations. Successful assertions and raw violation formatting omit it. + Start with `ArchUnit.project_files(project_locator = nil)`; `ArchUnit.files` is its short alias. Without a locator, ArchUnitRuby searches upward for a `Gemfile` or gemspec. diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c74829..6c9ff94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## 0.0.2 (unreleased) +- Add immutable `.because(...)` explanations to every executable rule and shared assertion output. + - Add opt-in debug inspection of project roots, graphs, and selected files on cold and cached checks. - Log passing metric values without repeating custom calculations. - Document verbosity, CI log artifacts, and existing colored violation reports. diff --git a/README.md b/README.md index a61bf9c..cd7189a 100644 --- a/README.md +++ b/README.md @@ -109,6 +109,23 @@ is required: ## 🚐 Setup +### Explain why a rule exists + +Append `.because(...)` to a completed file, layer, slice, or metric rule: + +~~~ ruby +rule = ArchUnit.project_files.in_folder('app/api/**') + .should_not.depend_on_files.in_folder('app/database/**') + .because('Keep persistence out of the API') + +expect(rule).to pass +~~~ + +The explanation appears once in a failed RSpec, Minitest, or `ArchUnit.assert_passes` assertion. +It is available as `rule.rationale`. Calling `.because(...)` returns a distinct frozen rule; +the original rule and direct `check` results are unchanged. Reasons must be non-blank strings. +Add the explanation after all selectors and modifiers. + ### Project Location The project locator is optional. With no argument, ArchUnitRuby searches from the current directory diff --git a/lib/archunit/common/fluentapi/checkable.rb b/lib/archunit/common/fluentapi/checkable.rb index b9796dd..2f1e8b9 100644 --- a/lib/archunit/common/fluentapi/checkable.rb +++ b/lib/archunit/common/fluentapi/checkable.rb @@ -11,6 +11,24 @@ module Common module FluentApi # The shared execution contract implemented by every terminal rule. module Checkable + # Optional normalized explanation, kept separate from violation values. + # @return [String, nil] + attr_reader :rationale + + # Explains why this completed rule exists, without changing the original. + # Append this after all selectors and modifiers. + # @param reason [String] a non-blank human-readable explanation + # @return [Checkable] a distinct frozen rule of the same type + def because(reason) + unless reason.is_a?(String) && !reason.strip.empty? + raise ArgumentError, 'reason must be a non-empty String' + end + + copy = dup + copy.instance_variable_set(:@rationale, reason.strip.freeze) + copy.freeze + end + # Executes the rule and returns every architecture disagreement as structured data. # @param options [CheckOptions, nil] # @return [Array] diff --git a/lib/archunit/testing.rb b/lib/archunit/testing.rb index fbca7a4..42d9a3c 100644 --- a/lib/archunit/testing.rb +++ b/lib/archunit/testing.rb @@ -28,9 +28,7 @@ def result_for(rule, options = nil, expected_to_pass: true) end violations = rule.check(options) - return ResultFactory.from_violations(violations) if expected_to_pass - - ResultFactory.from_violations(violations, expected_to_pass: false) + ResultFactory.from_violations(violations, expected_to_pass:, rationale: rule.rationale) end end diff --git a/lib/archunit/testing/result_factory.rb b/lib/archunit/testing/result_factory.rb index f5c99a7..b1df8e3 100644 --- a/lib/archunit/testing/result_factory.rb +++ b/lib/archunit/testing/result_factory.rb @@ -11,17 +11,29 @@ module Testing # Shapes a violation collection into one framework-neutral test result. class ResultFactory class << self - def from_violations(violations, color: nil, expected_to_pass: true) + def from_violations(violations, color: nil, expected_to_pass: true, rationale: nil) validate_violations(violations) validate_expected_to_pass(expected_to_pass) color = resolve_color(color) - return empty_result(color, expected_to_pass) if violations.empty? - - failing_result(violations, color, expected_to_pass) + result = if violations.empty? + empty_result(color, expected_to_pass) + else + failing_result(violations, color, expected_to_pass) + end + explain_result(result, rationale) end private + def explain_result(result, rationale) + unless rationale.nil? || (rationale.is_a?(String) && !rationale.strip.empty?) + raise ArgumentError, 'rationale must be a non-empty String or nil' + end + return result unless rationale && result.failed? + + TestResult.new(passed: false, message: "#{result.message}\n\nBecause: #{rationale.strip}") + end + def passing_result(color) message = ColorUtils.green('No architecture violations found.', enabled: color) TestResult.new(passed: true, message:) diff --git a/spec/common/fluentapi/rule_rationale_spec.rb b/spec/common/fluentapi/rule_rationale_spec.rb new file mode 100644 index 0000000..d9389a9 --- /dev/null +++ b/spec/common/fluentapi/rule_rationale_spec.rb @@ -0,0 +1,149 @@ +# frozen_string_literal: true + +require 'fileutils' +require 'minitest' +require 'pathname' +require 'tmpdir' + +RSpec.describe 'Rule rationale' do + around do |example| + Dir.mktmpdir('archunit-rationale') do |directory| + @root = Pathname.new(directory).realpath + @root.join('Gemfile').write('') + FileUtils.mkdir_p(@root.join('app/api')) + FileUtils.mkdir_p(@root.join('app/database')) + @root.join('app/api/request.rb').write("require_relative '../database/store'\n") + @root.join('app/database/store.rb').write('class Store; end') + ArchUnit.clear_graph_cache + example.run + ensure + ArchUnit.clear_graph_cache + end + end + + let(:reason) { 'Keep persistence out of the API' } + let(:rule) do + ArchUnit.project_files(@root).in_folder('app/api/**') + .should_not.depend_on_files.in_folder('app/database/**') + end + + # rubocop:disable-next Metrics/AbcSize, Metrics/MethodLength -- One fixture covers every terminal. + def all_terminals + files = ArchUnit.project_files(@root) + metrics = ArchUnit.metrics(@root) + slices = ArchUnit.project_slices(@root).defined_by('app/(**)/') + [ + files.should.have_name('*.rb'), + files.should.have_no_cycles, + files.should_not.depend_on_files.in_folder('app/database/**'), + files.should_not.depend_on_external_modules.matching('json'), + files.should.adhere_to(->(_file) { true }, 'Ruby files'), + ArchUnit.project_layers(@root).layer('api').defined_by('app/api/**') + .where_layer('api').may_only_depend_on_layers, + slices.should_not.contain_dependency('api', 'database'), + slices.should.adhere_to_diagram(slices.to_plantuml), + metrics.count.classes.should_be_above(0), + metrics.count.classes.should_satisfy(->(value, _info) { value >= 0 }), + metrics.custom_metric('members', 'member count', ->(info) { info.methods.length }) + .should_satisfy(->(value, _info) { value >= 0 }), + metrics.distance.not_in_zone_of_pain + ] + end + + it 'supports every concrete terminal through one immutable common contract' do + terminals = all_terminals + classes = ObjectSpace.each_object(Class).select do |candidate| + candidate.name&.start_with?('ArchUnit::') && candidate.include?(ArchUnit::Checkable) + end + expect(terminals.map(&:class).uniq).to match_array(classes) + + terminals.each do |original| + explained = original.because(" #{reason} ") + expect(explained).to be_a(original.class) + expect(explained).not_to equal(original) + expect(explained).to be_frozen + expect(explained.rationale).to eq(reason) + expect(explained.rationale).to be_frozen + expect(original.rationale).to be_nil + expect(ArchUnit.format_violations(explained.check, color: false)) + .to eq(ArchUnit.format_violations(original.check, color: false)) + end + end + + it 'owns its normalized string, supports replacement, and leaves caller values reusable' do + input = " #{reason} " + explained = rule.because(input) + input.replace('changed') + replacement = explained.because('A different explanation') + + expect(explained.rationale).to eq(reason) + expect(replacement.rationale).to eq('A different explanation') + expect(rule.rationale).to be_nil + end + + it 'rejects blank and non-string reasons' do + [nil, false, 42, :reason, '', " \t\n "].each do |invalid| + expect { rule.because(invalid) }.to raise_error(ArgumentError, /non-empty String/) + end + end + + it 'does no analysis while copying a rule' do + rule.check + expect(Prism).not_to receive(:parse) + expect(ArchUnit::Extraction).not_to receive(:extract_graph) + explained = rule.because(reason) + expect(explained.rationale).to eq(reason) + end + + it 'preserves graph cache identity and structured violations' do + violations = rule.check + graph = ArchUnit::Extraction.extract_graph(@root) + explained = rule.because(reason) + expect(explained.check).to eq(violations) + expect(ArchUnit::Extraction.extract_graph(@root)).to equal(graph) + expect(ArchUnit.format_violations(violations)).not_to include(reason) + end + + it 'includes the reason exactly once in framework-neutral and RSpec failures' do + explained = rule.because(reason) + expect { ArchUnit.assert_passes(explained) } + .to raise_error(ArchUnit::AssertionFailure) do |error| + expect(error.message.scan(reason).length).to eq(1) + end + expect { expect(explained).to pass } + .to raise_error(RSpec::Expectations::ExpectationNotMetError) do |error| + expect(error.message.scan(reason).length).to eq(1) + end + expect(explained).not_to pass + end + + it 'includes the reason exactly once in Minitest failures' do + ArchUnit::Testing::MinitestAdapter.install! + context = Class.new do + include Minitest::Assertions + + attr_accessor :assertions + end.new + context.assertions = 0 + + expect { context.assert_passes(rule.because(reason)) } + .to raise_error(Minitest::Assertion) do |error| + expect(error.message.scan(reason).length).to eq(1) + end + expect(context.assertions).to eq(1) + end + + it 'explains failed negated expectations but not successful assertions' do + passing = ArchUnit.project_files(@root).should.have_no_cycles.because(reason) + expect(passing).to pass + expect(ArchUnit.assert_passes(passing)).to be_nil + expect(ArchUnit::Testing.result_for(passing).message).not_to include(reason) + expect(ArchUnit::Testing.result_for(rule.because(reason), expected_to_pass: false).message) + .not_to include(reason) + expect { expect(passing).not_to pass } + .to raise_error(RSpec::Expectations::ExpectationNotMetError) do |error| + expect(error.message).to include('Expected architecture violations') + expect(error.message.scan(reason).length).to eq(1) + end + end +end diff --git a/spec/testing/assert_passes_spec.rb b/spec/testing/assert_passes_spec.rb index a82d0e1..1d390e4 100644 --- a/spec/testing/assert_passes_spec.rb +++ b/spec/testing/assert_passes_spec.rb @@ -73,7 +73,7 @@ def create_file(relative_path, contents = '# fixture') rule = rule_class.new expect(ArchUnit::ResultFactory).to receive(:from_violations) - .with([violation]).and_call_original + .with([violation], expected_to_pass: true, rationale: nil).and_call_original expect { ArchUnit.assert_passes(rule) }.to raise_error(ArchUnit::AssertionFailure) end diff --git a/spec/testing/result_factory_spec.rb b/spec/testing/result_factory_spec.rb index cfbba62..c4c9452 100644 --- a/spec/testing/result_factory_spec.rb +++ b/spec/testing/result_factory_spec.rb @@ -74,4 +74,11 @@ described_class.from_violations([violation], color: false).message ) end + + it 'validates optional rationale independently of the result' do + ['', ' ', false, 42].each do |invalid| + expect { described_class.from_violations([], rationale: invalid) } + .to raise_error(ArgumentError, /rationale/) + end + end end