Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
24 changes: 23 additions & 1 deletion USAGE.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,28 @@ Packwerk reads from the `packwerk.yml` configuration file in the root directory.
| cache | false | when true, caches the results of parsing files |
| cache_directory | tmp/cache/packwerk | the directory that will hold the packwerk cache |

### Using custom parsers

You can specify a custom parser to parse different file formats (e.g. slim or haml)

```ruby
class SlimParser
include ParserInterface
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnamsons In some of the other merged PRs, we ended up just going with the plain Checker and Validator as names for the interfaces to be included to extend packwerk. Can we just call this Parser, i.e. include Packwerk::Parser

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense 👍
the Ruby parser also defines the Parser namespace, to avoid confusion, I added Packerk:: to every usage of this interface.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should rename to something like FileParser?


REGEX = /\.slim\Z/

def call
# Your parsing logic here
end

def match?(path)
REGEX.match?(path)
end
end

Packwerk::Parsers::Factory.instance.parsers.append(SlimParser)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of asking the user to explicitly specify these parsers, can we have them included automatically?

Here's what I had in mind:

# typed: strict
# frozen_string_literal: true

module Packwerk
  module Parsers
    module ParserInterface
      extend T::Helpers
      extend T::Sig

      requires_ancestor { Kernel }

      interface!

      sig { params(base: Class).void }
      def self.included(base)
        @parsers ||= T.let(@parsers, T.nilable(T::Array[Class]))
        @parsers ||= []
        @parsers << base
      end

      sig { returns(T::Array[ParserInterface]) }
      def self.all
        T.unsafe(@parsers).map(&:new)
      end

      sig { abstract.params(io: File, file_path: String).returns(T.untyped) }
      def call(io:, file_path:)
      end
    end
  end
end

Then, the user just needs to define this in a file that is "required" within packwerk.yml via this new API in this upcoming PR: #245

Elsewhere, we can just check Packwerk::Parsers::ParserInterface.all to get the list of all of the parsers.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is certainly a better approach than what I had created, as it makes adding new parsers easier 👍

The only concern i have is that the method Packwerk::Parsers::ParserInterface.all seems like it's not cohesive to the idea of a parser interface, to me it would seem more appropriate if we were to call Packwerk::Parsers.all and be able to get all of the registered parsers. What do you think about something like this?

# lib/packwerk/parsers/parser_interface.rb
module Packwerk
  module Parsers
    module ParserInterface
      extend T::Helpers
      extend T::Sig

      requires_ancestor { Kernel }

      interface!

      sig { params(base: Class).void }
      def self.included(base)
        Packwerk::Parsers.register(base)
      end

      sig { abstract.params(io: File, file_path: String).returns(T.untyped) }
      def call(io:, file_path:)
      end

      sig { abstract.params(path: String).returns(T.nilable(ParserInterface)) }
      def match?(path)
      end
    end
  end
end
# lib/packwerk/parsers.rb
module Packwerk
  module Parsers
    autoload :Erb, "packwerk/parsers/erb"
    autoload :Factory, "packwerk/parsers/factory"
    autoload :ParserInterface, "packwerk/parsers/parser_interface"
    autoload :Ruby, "packwerk/parsers/ruby"

    sig { params(parser: ParserInterface).void }
    def self.register(parser)
      @parsers ||= T.let([], T.nilable(T::Array[ParserInterface]))
      
      @parsers << parser
    end

    sig { returns(T::Array[ParserInterface]) }
    def self.all
      T.unsafe(@parsers).map(&:new)
    end
  end
end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't imagine the ability to get all parsers to be a public API. I do think that's a cleaner internal API for packwerk though! However I'm not sure we'll want to do this for some of the other things we are able to or will be able to extend (offenses formatters, checkers) since there isn't an existing module for them (i.e. no standalone Formatters or Checkers modules), and I'm not sure if we'll want to add one just to hold the registered interfaces.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention wasn't to make the listing of parsers as a public API, just to make it a bit cleaner for internal use, but I can see that adding modules for Formatters and Checkers just to hold extensions might be a bit excessive.

I'll change my Pull request to use the solution that You outlined, so we don't have differing APIs for how Parsers get stored vs. Formatters & Checkers.

```

### Using a custom ERB parser

You can specify a custom ERB parser if needed. For example, if you're using `<%graphql>` tags from https://github.com/github/graphql-client in your ERBs, you can use a custom parser subclass to comment them out so that Packwerk can parse the rest of the file:
Expand All @@ -99,7 +121,7 @@ class CustomParser < Packwerk::Parsers::Erb
end
end

Packwerk::Parsers::Factory.instance.erb_parser_class = CustomParser
Packwerk::Parsers::Factory.instance.parsers = [Packwerk::Parsers::Ruby, CustomParser]
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexevanczuk After these changes, I don't see an easy way to override the erb parser.

IMO the best approach would be to move the erb parser to a seperate gem, but if we want to keep it in packwerk and have a way of configuring it, then I will add a configuration for this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share more about why you want to override the ERB parser? I know we want to add new parsers, but why override the other one?

Separately, I think if we want to extract ERB, https://github.com/rubyatscale/packwerk-extensions would be a great home for it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexevanczuk I don't have a particular use case, but the API for overriding the erb parser was there before my changes(https://github.com/Shopify/packwerk/blob/main/USAGE.md#using-a-custom-erb-parser) so I was thinking about how to keep feature parity.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting... I see a couple of options:

  1. We could keep the existing API exactly as is and implement it by having an override when getting the parser to use (the override would check if the parser to fetch is ERB and use the overridden value if provided). The advantages here are we don't have to think about changing the API at all.
  2. We could eliminate this API entirely and when someone wants to custom parse an ERB, they could just pass in their own custom ERB parser that parsers out things the default parser cannot. Advantage here is that it's one less "extension" API to support.

What do you think @dnamsons and @gmcgibbon?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnamsons I took another look at this. What if we changed this line:
https://github.com/Shopify/packwerk/pull/243/files#diff-79f5d7b9ceb194cd2ea7989b1a3839f0c8f8d14cdcd2ff7dfe02299498a82648R14

To select instead of find the parsers, which will allow multiple parsers to parse the same file? That way, someone can "extend" how any type of file is being parsed (ruby, erb, etc.)?

Then we can remove this API and its behavior can be ported to an ERB parser extension.

```

## Using the cache
Expand Down
7 changes: 7 additions & 0 deletions lib/packwerk/parsers/erb.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ module Parsers
class Erb
include ParserInterface

ERB_REGEX = /\.erb\Z/
private_constant :ERB_REGEX

def initialize(parser_class: BetterHtml::Parser, ruby_parser: Ruby.new)
@parser_class = parser_class
@ruby_parser = ruby_parser
Expand All @@ -33,6 +36,10 @@ def parse_buffer(buffer, file_path:)
raise Parsers::ParseError, result
end

def self.match?(path)
ERB_REGEX.match?(path)
end

private

def to_ruby_ast(erb_ast, file_path)
Expand Down
36 changes: 13 additions & 23 deletions lib/packwerk/parsers/factory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,34 +9,24 @@ class Factory
extend T::Sig
include Singleton

RUBY_REGEX = %r{
# Although not important for regex, these are ordered from most likely to match to least likely.
\.(rb|rake|builder|gemspec|ru)\Z
|
(Gemfile|Rakefile)\Z
}x
private_constant :RUBY_REGEX
DEFAULT_PARSERS = T.let([
Ruby,
Erb,
], T::Array[ParserInterface])

ERB_REGEX = /\.erb\Z/
private_constant :ERB_REGEX
sig { returns(T::Array[ParserInterface]) }
attr_accessor :parsers

sig { params(path: String).returns(T.nilable(ParserInterface)) }
def for_path(path)
case path
when RUBY_REGEX
@ruby_parser ||= Ruby.new
when ERB_REGEX
@erb_parser ||= erb_parser_class.new
end
sig { void }
def initialize
@parsers = T.let(DEFAULT_PARSERS, T::Array[ParserInterface])
end

def erb_parser_class
@erb_parser_class ||= Erb
end
sig { params(path: String).returns(T.nilable(ParserInterface)) }
def for_path(path)
parser_for_path = parsers.find { |parser| parser.match?(path) }

def erb_parser_class=(klass)
@erb_parser_class = klass
@erb_parser = nil
parser_for_path&.new
end
end
end
Expand Down
4 changes: 4 additions & 0 deletions lib/packwerk/parsers/parser_interface.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ module ParserInterface
sig { abstract.params(io: File, file_path: String).returns(T.untyped) }
def call(io:, file_path:)
end

sig { abstract.params(path: String).returns(T.boolean) }
def self.match?(path:)
end
end
end
end
12 changes: 12 additions & 0 deletions lib/packwerk/parsers/ruby.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ module Parsers
class Ruby
include ParserInterface

RUBY_REGEX = %r{
# Although not important for regex, these are ordered from most likely to match to least likely.
\.(rb|rake|builder|gemspec|ru)\Z
|
(Gemfile|Rakefile)\Z
}x
private_constant :RUBY_REGEX

class RaiseExceptionsParser < Parser::CurrentRuby
def initialize(builder)
super(builder)
Expand Down Expand Up @@ -39,6 +47,10 @@ def call(io:, file_path: "<unknown>")
result = ParseResult.new(file: file_path, message: "Syntax error: #{e}")
raise Parsers::ParseError, result
end

def self.match?(path)
RUBY_REGEX.match?(path)
end
end
end
end
18 changes: 12 additions & 6 deletions test/unit/parsers/factory_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,19 @@ class FactoryTest < Minitest::Test
assert_instance_of(Parsers::Erb, factory.for_path("foo.html.erb"))
assert_instance_of(Parsers::Erb, factory.for_path("foo.md.erb"))
assert_instance_of(Parsers::Erb, factory.for_path("/sub/directory/foo.erb"))
end

fake_class = Class.new do
test "#for_path gives custom parser for paths the parser is defined to match" do
fake_erb_parser_class = Class.new do
T.unsafe(self).include(ParserInterface)

def self.match?(path)
/\.erb\Z/.match?(path)
end
end

with_erb_parser_class(fake_class) do
assert_instance_of(fake_class, factory.for_path("foo.html.erb"))
with_parser_class(fake_erb_parser_class) do
assert_instance_of(fake_erb_parser_class, factory.for_path("foo.html.erb"))
end
end

Expand All @@ -41,11 +47,11 @@ class FactoryTest < Minitest::Test

private

def with_erb_parser_class(klass)
factory.erb_parser_class = klass
def with_parser_class(klass)
factory.parsers = [klass]
yield
ensure
factory.erb_parser_class = nil
factory.parsers = Parsers::Factory::DEFAULT_PARSERS
end

def factory
Expand Down