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

alexevanczuk · 2022-11-20T13:23:45Z

What are you trying to accomplish?

Today, packwerk supports a configurable OffensesFormatter. However, the only way to configure it is to load the CLI itself. This means that this feature is not usable if a user has run bundle binstubs packwerk. Instead, the user needs to copy exe/packwerk into their local bin/packwerk and pass in a different formatter to Packwerk::Cli.new.

This PR makes this more user-friendly by leveraging the extension system to load in a new formatter. This way, a user can use the vanilla packwerk binstub and load in their own offenses formatter more easily.

This was originally attempted here: #264
However, I believe both the implementation and the UX of this approach is better and more consistent.

What approach did you choose and why?

I chose to use the require directive in packwerk.yml to load in a formatter via a setting on Packwerk::Configuration. The system first prioritizes the local input variable, then the configuration, then the default.

I chose to use a similar approach as the Checker and Validator to keep things consistent. That is, by requiring a file that implements the interface, the extension is "loaded." Then to turn "on" the extension, the user needs to set it (via packwerk.yml).

What should reviewers focus on?

There are some subtle changes that I've pointed out via comments. Looking for feedback on it!

Type of Change

Bugfix
New feature
Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Additional Release Notes

Breaking change (fix or feature that would cause existing functionality to change)

You could definitely argue that this is a breaking change because I change the interface to OffensesFormatter. It now requires an identifier. I also removed the initialize method that passes in a style so that the formatters can be initialized consistently. Therefore those using the OffensesFormatter directly will experience a breaking API change.

Checklist

I have updated the documentation accordingly.
I have added tests to cover my changes.
It is safe to rollback this change.

USAGE.md

lib/packwerk/cli.rb

lib/packwerk/configuration.rb

lib/packwerk/formatters/offenses_formatter.rb

lib/packwerk/formatters/offenses_formatter_plain.rb

lib/packwerk/offenses_formatter.rb

USAGE.md

lib/packwerk/formatters/offenses_formatter_plain.rb

USAGE.md

lib/packwerk/configuration.rb

lib/packwerk/offenses_formatter.rb

alexevanczuk · 2022-11-22T19:23:42Z

@gmcgibbon I think I've addressed/responded to all of your feedback.

alexevanczuk · 2022-11-22T19:44:57Z

@gmcgibbon I pushed a command line option, as I realized that without it, there is no path for a user to use different formatters in different options without modifying packwerk.yml, so it felt essential.

USAGE.md

lib/packwerk/offenses_formatter.rb

USAGE.md

lib/packwerk/formatters/offenses_formatter.rb

gmcgibbon · 2022-11-24T23:19:41Z

lib/packwerk/offenses_formatter.rb

+      private
+
+      sig { void }
+      def load_default_formatter


There isn't a need to referencer the constant if we require the offense formatter at the top of the file. If the formatter is only used when checking, maybe the best thing to do would be to do a runtime require "packwerk/formatters/offenses_formatter" in this function?

Sure, I like that! Definitely clearer that we're explicitly loading rather than loading through invoking auto load.

gmcgibbon · 2022-11-24T23:22:01Z

test/support/application_fixture_helper.rb

@@ -13,6 +13,11 @@ def setup_application_fixture
    @old_working_dir = Dir.pwd
  end

+  def remove_extensions
+    Object.send(:remove_const, :MyLocalExtension) if defined?(MyLocalExtension)


Suggested change

Object.send(:remove_const, :MyLocalExtension) if defined?(MyLocalExtension)

Object.send(:remove_const, :MyLocalExtension)

If we expect it to be there, it should be there. Otherwise our test is wrong for calling this method.

Sure makes sense! Will change.

gmcgibbon

LGTM after some small changes.

USAGE.md

lib/packwerk/cli.rb

test/test_helper.rb

gmcgibbon · 2022-11-26T02:43:46Z

test/support/application_fixture_helper.rb

+    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)


We could just leave it at setting nil, right?

We can't because @offenses_formatters is initialized when the interface is included when a file is required, so if we reset to nil in the test suite, then no more offense formatter will be required, so nil.map will raise an error.

lib/packwerk/offenses_formatter.rb

gmcgibbon · 2022-11-26T02:46:45Z

USAGE.md

+
+You can also pass in a formatter on the command line:
+```
+bin/packwerk check --format=my_offenses_formatter


Make sure this is using the correct flag before merging.

alexevanczuk force-pushed the ae-extensible-formatter-take-2 branch from 02e5ce1 to 9dac93b Compare November 20, 2022 13:30