Add ability to load in extensions to packwerk #245

alexevanczuk · 2022-10-27T19:26:22Z

Commits

Have FilesForProcessingTest use application fixture pattern
Add failing test
Make tests pass

What are you trying to accomplish?

As packwerk enables more types of configuration, we need an easy to use and consistent experience for clients trying to extend packwerk. We have three things that we want to be able to extend:

The offenses formatter, which is currently not extensible via the command line (without altering the binstub)
Custom parsers
Custom checkers

Ideally, the user can use a consistent pattern to define their extensions, such as defining a method that inherits a packwerk Sorbet interface. Regardless of what that interface is (this PR does not decide that), the user will need a way to have that code load when packwerk is executed. This PR creates this ability by requiring files as determined by the user in packwerk.yml.

What approach did you choose and why?

Since packwerk is a static analysis tool, we used rubocop as inspiration for this API:
https://docs.rubocop.org/rubocop/extensions.html#loading-extensions

What should reviewers focus on?

Let me know if there's another approach that makes more sense to you!

Type of Change

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

Checklist

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

alexevanczuk · 2022-10-27T19:27:23Z

USAGE.md

@@ -275,3 +276,16 @@ Above is an example of a constant violation entry in `deprecated_references.yml`
 * `components/merchant/app/public/merchant/generate_order.rb` - path to the file containing the violated constant

 Violations exist within the package that makes a violating reference. This means privacy violations of your package can be found listed in `deprecated_references.yml` files in the packages with the reference to a private constant.
+
+# Loading Extensions


Later, when we support offenses formatter, checkers, and parsers to be configured using a consistent API, we can add more information about how to configure these within this section.

USAGE.md

alexevanczuk · 2022-10-28T20:59:28Z

Applied your suggestions @gmcgibbon ! Thank you for using the suggestions feature – made it really easy to respond to code review 🙏

gmcgibbon · 2022-10-31T17:15:14Z

test/fixtures/skeleton/config/my_extension.rb

@@ -0,0 +1,2 @@
+module MySpecialExtension


Suggested change

module MySpecialExtension

module MyLocalExtension

I would also rename the file.

gmcgibbon · 2022-10-31T17:16:39Z

test/unit/configuration_test.rb

+      refute defined?(MySpecialExtension)
+      Configuration.from_path
+      assert defined?(MySpecialExtension)
+    end


Suggested change

end

ensure

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

end

You need to clean up globals after the test is ran to make it not have side effects for other tests.

Yes! Thank you.

gmcgibbon · 2022-10-31T17:19:01Z

lib/packwerk/configuration.rb

@@ -49,6 +49,12 @@ def initialize(configs = {}, config_path: nil)
      @cache_directory = Pathname.new(configs["cache_directory"] || "tmp/cache/packwerk")
      @config_path = config_path

+      if configs.key?("require")
+        configs["require"].each do |require_directive|
+          ExtensionLoader.load(require_directive, @root_path)


I'm trying to understand why we need a module to do this. Why can't we just use require and assume relative paths are from the current working directory?

We don't necessarily need a module – I just wanted to encapsulate its logic into one place. I made it a private module with private_constant. I also thought that it could potentially expand, so wanted to make room for that.

Let me know if you'd prefer to inline as private methods

True. You've changed my mind, this is worth keeping.

gmcgibbon · 2022-10-31T17:19:27Z

lib/packwerk/extension_loader.rb

+module Packwerk
+  # This class handles loading extensions to packwerk using the `require` directive
+  # in the `packwerk.yml` configuration.
+  class ExtensionLoader


Suggested change

class ExtensionLoader

module ExtensionLoader

gmcgibbon · 2022-10-31T17:20:57Z

test/unit/configuration_test.rb

+      use_template(:skeleton)
+
+      required_things = []
+      require_method = ->(required_thing) do


Suggested change

require_method = ->(required_thing) do

mock_require_method = ->(required_thing) do

You could technically just check $LOADED_FEATURES, but I think is is cleaner 👍

gmcgibbon · 2022-10-31T17:48:23Z

test/fixtures/skeleton/packwerk.yml

@@ -4,3 +4,6 @@ include:
 exclude:
  - "**/temp.rb"
 parallel: false
+require:
+  - ./config/my_local_extension.rb


Suggested change

- ./config/my_local_extension.rb

- ./config/my_local_extension

Good call, changed

gmcgibbon · 2022-10-31T17:49:43Z

test/fixtures/skeleton/packwerk.yml

@@ -4,3 +4,6 @@ include:
 exclude:
  - "**/temp.rb"
 parallel: false
+require:
+  - ./config/my_local_extension.rb
+  - pathname


Suggested change

- pathname

- my_gem_extension

Personally I think this would make more sense, but it breaks existing tests I think. This probably means we should make a new fixture since I wouldn't expect the skeleton / barebones fixture to reference extensions. Let's do this before merging.

Yep, I agree. I'll make a new fixture.

@gmcgibbon You approved this already, but I just want to double check that my change is what you had in mind. I created a new fixture extended which let me better colocate test concerns related to extending packwerk.

Let me know if you're still good with it, in which case I'll squash the commits and merge in the PR.

alexevanczuk requested a review from a team as a code owner October 27, 2022 19:26

alexevanczuk commented Oct 27, 2022

View reviewed changes

alexevanczuk force-pushed the ae-add-require-directive branch from efa1e25 to 53c259e Compare October 27, 2022 19:27

alexevanczuk mentioned this pull request Oct 27, 2022

Allow to configure parsers in packwerk #243

Open

7 tasks

alexevanczuk force-pushed the ae-add-require-directive branch 2 times, most recently from 8c0691f to 539950d Compare October 28, 2022 20:16

gmcgibbon reviewed Oct 28, 2022

View reviewed changes

USAGE.md Show resolved Hide resolved

USAGE.md Show resolved Hide resolved

USAGE.md Show resolved Hide resolved

USAGE.md Show resolved Hide resolved

USAGE.md Show resolved Hide resolved

alexevanczuk requested a review from gmcgibbon October 28, 2022 20:58

gmcgibbon reviewed Oct 31, 2022

View reviewed changes

alexevanczuk force-pushed the ae-add-require-directive branch from b7882b8 to 5a334c8 Compare October 31, 2022 17:29

alexevanczuk requested a review from gmcgibbon October 31, 2022 17:30

alexevanczuk force-pushed the ae-add-require-directive branch from 5a334c8 to 925e147 Compare October 31, 2022 17:39

gmcgibbon approved these changes Oct 31, 2022

View reviewed changes

alexevanczuk force-pushed the ae-add-require-directive branch from 4da8f4e to 353f217 Compare October 31, 2022 18:03

gmcgibbon approved these changes Oct 31, 2022

View reviewed changes

Add ability to load extensions to packwerk

bd1c27b

alexevanczuk force-pushed the ae-add-require-directive branch from 353f217 to bd1c27b Compare October 31, 2022 19:46

alexevanczuk merged commit f906bb7 into main Oct 31, 2022

alexevanczuk deleted the ae-add-require-directive branch October 31, 2022 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to load in extensions to packwerk #245

Add ability to load in extensions to packwerk #245

alexevanczuk commented Oct 27, 2022

alexevanczuk Oct 27, 2022

alexevanczuk commented Oct 28, 2022

gmcgibbon Oct 31, 2022

alexevanczuk Oct 31, 2022

gmcgibbon Oct 31, 2022

alexevanczuk Oct 31, 2022

gmcgibbon Oct 31, 2022

alexevanczuk Oct 31, 2022

gmcgibbon Oct 31, 2022

gmcgibbon Oct 31, 2022

alexevanczuk Oct 31, 2022

gmcgibbon Oct 31, 2022

alexevanczuk Oct 31, 2022 •

edited

gmcgibbon Oct 31, 2022

alexevanczuk Oct 31, 2022

gmcgibbon Oct 31, 2022 •

edited

alexevanczuk Oct 31, 2022

alexevanczuk Oct 31, 2022

-    end
+    ensure
+      Object.send(:remove_const, :MyLocalExtension) if defined?(MyLocalExtension)
+    end

	require_method = ->(required_thing) do
	mock_require_method = ->(required_thing) do

	- ./config/my_local_extension.rb
	- ./config/my_local_extension

Add ability to load in extensions to packwerk #245

Add ability to load in extensions to packwerk #245

Conversation

alexevanczuk commented Oct 27, 2022

Commits

What are you trying to accomplish?

What approach did you choose and why?

What should reviewers focus on?

Type of Change

Checklist

Choose a reason for hiding this comment

alexevanczuk commented Oct 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexevanczuk Oct 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcgibbon Oct 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexevanczuk Oct 31, 2022 •

edited

gmcgibbon Oct 31, 2022 •

edited