Merge pull request #266 from Shopify/ae-extensible-formatter-take-2

Allow OffensesFormatter to be configured via packwerk.yml and CLI flag

USAGE.md

@@ -146,6 +146,7 @@ Example:
 Packwerk ships with dependency boundary checking only. See [`packwerk-extensions`](https://github.com/rubyatscale/packwerk-extensions) to incorporate privacy checks into your use of `packwerk`.
 
 #### Enforcing dependency boundary
+
 A package's dependency boundary is violated whenever it references a constant in some package that has not been declared as a dependency.
 
 Specify `enforce_dependencies: true` to start enforcing the dependencies of a package. The intentional dependencies of the package are specified as a list under a `dependencies:` key.
@@ -184,6 +185,7 @@ In order to keep the package system valid at each version of the application, we
 See: [TROUBLESHOOT.md - Sample violations](TROUBLESHOOT.md#Sample-violations)
 
 ## Resolving new violations
+
 ### Understanding how to respond to new violations
 
 When you have a new dependency or privacy violation, what do you do?
@@ -210,7 +212,6 @@ _Note: Changing dependencies or enabling dependencies will not require a full up
 
 See: [TROUBLESHOOT.md - Troubleshooting violations](TROUBLESHOOT.md#Troubleshooting_violations)
 
-
 ### Understanding the package todo file
 
 The package TODO list is called `package_todo.yml` and can be found in the package folder. The list outlines the constant violations of the package, where the violation is located, and the file defining the violation.
@@ -249,6 +250,34 @@ You can prefix local files with a dot to define them relative to `packwerk.yml`,
 You can also reference the name of a gem.
 
 ## Examples
+
+### Custom Offense Formatter
+
+While `packwerk` ships with its own offense formatter, you may specify a custom one in your configuration file via the `offenses_formatter:` key.  Your custom formatter will be used when `bin/packwerk check` is run.
+
+Firstly, you'll need to create an `OffensesFormatter` class that includes `Packwerk::OffensesFormatter`. You can use [`Packwerk::Formatters::OffensesFormatter`](lib/packwerk/formatters/offenses_formatter.rb) as a point of reference for this. Then, in the `require` directive described above, you'll want to tell `packwerk` about it:
+```ruby
+# ./path/to/file.rb
+class MyOffensesFormatter
+  include Packwerk::OffensesFormatter
+  # implement the `OffensesFormatter` interface
+
+  def identifier
+    'my_offenses_formatter'
+  end
+end
+```
+
+Then in `packwerk.yml`, you can set the `formatter` to the identifier for your class:
+```yml
+offenses_formatter: my_offenses_formatter
+```
+
+You can also pass in a formatter on the command line:
+```
+bin/packwerk check --offenses-formatter=my_offenses_formatter
+```
+
 ### Privacy Checker
 
 [`packwerk-extensions`](https://github.com/rubyatscale/packwerk-extensions) (originally extracted from `packwerk`) can be used to help define and enforce public API boundaries of a package. See the README.md for more details. To use this, add it to your `Gemfile` and then require it via `packwerk.yml`:

lib/packwerk.rb

@@ -64,7 +64,6 @@ module OutputStyles
   module Formatters
     extend ActiveSupport::Autoload
 
-    autoload :OffensesFormatter
     autoload :ProgressFormatter
   end
 

lib/packwerk/cli.rb

@@ -33,7 +33,7 @@ def initialize(
       @configuration = T.let(configuration || Configuration.from_path, Configuration)
       @progress_formatter = T.let(Formatters::ProgressFormatter.new(@out, style: style), Formatters::ProgressFormatter)
       @offenses_formatter = T.let(
-        offenses_formatter || Formatters::OffensesFormatter.new(style: @style),
+        offenses_formatter || @configuration.offenses_formatter,
         OffensesFormatter
       )
     end
@@ -182,6 +182,7 @@ def list_validation_errors(result)
     def parse_run(params)
       relative_file_paths = T.let([], T::Array[String])
       ignore_nested_packages = nil
+      formatter = @offenses_formatter
 
       if params.any? { |p| p.include?("--packages") }
         OptionParser.new do |parser|
@@ -195,11 +196,20 @@ def parse_run(params)
         ignore_nested_packages = false
       end
 
+      if params.any? { |p| p.include?("--offenses-formatter") }
+        OptionParser.new do |parser|
+          parser.on("--offenses-formatter=FORMATTER", String,
+            "identifier of offenses formatter to use") do |formatter_identifier|
+            formatter = OffensesFormatter.find(formatter_identifier)
+          end
+        end.parse!(params)
+      end
+
       ParseRun.new(
         relative_file_set: fetch_files_to_process(relative_file_paths, ignore_nested_packages),
         configuration: @configuration,
         progress_formatter: @progress_formatter,
-        offenses_formatter: @offenses_formatter
+        offenses_formatter: formatter
       )
     end
   end

lib/packwerk/configuration.rb

@@ -6,6 +6,8 @@
 
 module Packwerk
   class Configuration
+    extend T::Sig
+
     class << self
       def from_path(path = Dir.pwd)
         raise ArgumentError, "#{File.expand_path(path)} does not exist" unless File.exist?(path)
@@ -49,6 +51,8 @@ def initialize(configs = {}, config_path: nil)
       @cache_directory = Pathname.new(configs["cache_directory"] || "tmp/cache/packwerk")
       @config_path = config_path
 
+      @offenses_formatter_identifier = configs["offenses_formatter"] || Formatters::OffensesFormatter::IDENTIFIER
+
       if configs.key?("require")
         configs["require"].each do |require_directive|
           ExtensionLoader.load(require_directive, @root_path)
@@ -64,6 +68,11 @@ def parallel?
       @parallel
     end
 
+    sig { returns(OffensesFormatter) }
+    def offenses_formatter
+      OffensesFormatter.find(@offenses_formatter_identifier)
+    end
+
     def cache_enabled?
       @cache_enabled
     end

lib/packwerk/formatters/offenses_formatter.rb

@@ -6,12 +6,9 @@ module Formatters
     class OffensesFormatter
       include Packwerk::OffensesFormatter
 
-      extend T::Sig
+      IDENTIFIER = T.let("default", String)
 
-      sig { params(style: OutputStyle).void }
-      def initialize(style: OutputStyles::Plain.new)
-        @style = style
-      end
+      extend T::Sig
 
       sig { override.params(offenses: T::Array[T.nilable(Offense)]).returns(String) }
       def show_offenses(offenses)
@@ -32,13 +29,23 @@ def show_stale_violations(offense_collection, fileset)
         end
       end
 
+      sig { override.returns(String) }
+      def identifier
+        IDENTIFIER
+      end
+
       private
 
+      sig { returns(OutputStyle) }
+      def style
+        @style ||= T.let(Packwerk::OutputStyles::Coloured.new, T.nilable(Packwerk::OutputStyles::Coloured))
+      end
+
       sig { params(offenses: T::Array[T.nilable(Offense)]).returns(String) }
       def offenses_list(offenses)
         offenses
           .compact
-          .map { |offense| offense.to_s(@style) }
+          .map { |offense| offense.to_s(style) }
           .join("\n")
       end
 

lib/packwerk/offenses_formatter.rb

@@ -6,7 +6,57 @@ module OffensesFormatter
     extend T::Sig
     extend T::Helpers
 
-    interface!
+    abstract!
+
+    class DuplicateFormatterError < StandardError
+      extend T::Sig
+
+      sig { params(identifier: String).void }
+      def initialize(identifier)
+        super("Cannot have multiple identifiers with the same key (`#{identifier}`)")
+      end
+    end
+
+    class << self
+      extend T::Sig
+
+      sig { params(base: Class).void }
+      def included(base)
+        @offenses_formatters ||= T.let(@offenses_formatters, T.nilable(T::Array[Class]))
+        @offenses_formatters ||= []
+        @offenses_formatters << base
+      end
+
+      sig { returns(T::Array[OffensesFormatter]) }
+      def all
+        require "packwerk/formatters/offenses_formatter"
+        T.unsafe(@offenses_formatters).map(&:new)
+      end
+
+      sig { params(identifier: String).returns(OffensesFormatter) }
+      def find(identifier)
+        formatter_by_identifier(identifier)
+      end
+
+      private
+
+      sig { params(name: String).returns(OffensesFormatter) }
+      def formatter_by_identifier(name)
+        @formatter_by_identifier ||= T.let(nil, T.nilable(T::Hash[String, T.nilable(OffensesFormatter)]))
+        @formatter_by_identifier ||= begin
+          index = T.let({}, T::Hash[String, T.nilable(OffensesFormatter)])
+          OffensesFormatter.all.each do |formatter|
+            identifier = formatter.identifier
+            raise DuplicateFormatterError, identifier if index.key?(identifier)
+
+            index[identifier] = formatter
+          end
+          T.let(index, T.nilable(T::Hash[String, T.nilable(OffensesFormatter)]))
+        end
+
+        T.must(T.must(@formatter_by_identifier)[name])
+      end
+    end
 
     sig { abstract.params(offenses: T::Array[T.nilable(Offense)]).returns(String) }
     def show_offenses(offenses)
@@ -15,5 +65,9 @@ def show_offenses(offenses)
     sig { abstract.params(offense_collection: Packwerk::OffenseCollection, for_files: T::Set[String]).returns(String) }
     def show_stale_violations(offense_collection, for_files)
     end
+
+    sig { abstract.returns(String) }
+    def identifier
+    end
   end
 end

lib/packwerk/parse_run.rb

@@ -16,19 +16,20 @@ class ParseRun
       params(
         relative_file_set: FilesForProcessing::RelativeFileSet,
         configuration: Configuration,
+        offenses_formatter: T.nilable(OffensesFormatter),
         progress_formatter: Formatters::ProgressFormatter,
-        offenses_formatter: OffensesFormatter,
       ).void
     end
     def initialize(
       relative_file_set:,
       configuration:,
-      progress_formatter: Formatters::ProgressFormatter.new(StringIO.new),
-      offenses_formatter: Formatters::OffensesFormatter.new
+      offenses_formatter: nil,
+      progress_formatter: Formatters::ProgressFormatter.new(StringIO.new)
     )
+
       @configuration = configuration
       @progress_formatter = progress_formatter
-      @offenses_formatter = offenses_formatter
+      @offenses_formatter = T.let(offenses_formatter || configuration.offenses_formatter, Packwerk::OffensesFormatter)
       @relative_file_set = relative_file_set
     end
 

test/fixtures/extended/config/my_local_extension.rb

@@ -1,2 +1,18 @@
 module MyLocalExtension
 end
+
+class MyOffensesFormatter
+  include Packwerk::OffensesFormatter
+
+  def show_offenses(offenses)
+    ["hi i am a custom offense formatter", *offenses].join("\n")
+  end
+
+  def show_stale_violations(_offense_collection, _fileset)
+    "stale violations report"
+  end
+
+  def identifier
+    'my_offenses_formatter'
+  end
+end

test/fixtures/extended/packwerk.yml

@@ -2,3 +2,4 @@ parallel: false
 require:
   - ./config/my_local_extension
   - my_gem_extension
+offenses_formatter: my_offenses_formatter

test/fixtures/minimal/packwerk.yml

@@ -1,2 +1,4 @@
 include:
   - "**/*.rb"
+
+offenses_formatter: plain

test/support/application_fixture_helper.rb

@@ -13,6 +13,18 @@ def setup_application_fixture
     @old_working_dir = Dir.pwd
   end
 
+  def remove_extensions
+    Object.send(:remove_const, :MyLocalExtension)
+    reset_formatters
+  end
+
+  def reset_formatters
+    Packwerk::OffensesFormatter.instance_variable_set(:@formatter_by_identifier, nil)
+    current_formatters = Packwerk::OffensesFormatter.instance_variable_get(:@offenses_formatters)
+    new_formatters = current_formatters.delete_if { |f| f.new.identifier == "my_offenses_formatter" }
+    Packwerk::OffensesFormatter.instance_variable_set(:@offenses_formatters, new_formatters)
+  end
+
   def teardown_application_fixture
     Dir.chdir(@old_working_dir)
     FileUtils.remove_entry(@app_dir, true) if using_template?

test/support/offenses_formatter_plain.rb

@@ -0,0 +1,27 @@
+# typed: strict
+# frozen_string_literal: true
+
+require "packwerk/formatters/offenses_formatter"
+
+module Packwerk
+  module Formatters
+    class OffensesFormatterPlain < OffensesFormatter
+      include Packwerk::OffensesFormatter
+      IDENTIFIER = T.let("plain", String)
+
+      extend T::Sig
+
+      sig { override.returns(String) }
+      def identifier
+        IDENTIFIER
+      end
+
+      private
+
+      sig { returns(OutputStyle) }
+      def style
+        @style ||= T.let(Packwerk::OutputStyles::Plain.new, T.nilable(Packwerk::OutputStyle))
+      end
+    end
+  end
+end

test/test_helper.rb

@@ -19,6 +19,7 @@
 require "support/test_assertions"
 require "support/yaml_file"
 require "support/typed_mock"
+require "support/offenses_formatter_plain"
 
 Minitest::Test.include(StubConst)
 Minitest::Test.extend(TestMacro)