-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow to configure parsers in packwerk #243
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Elsewhere, we can just check There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 # 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I'll change my Pull request to use the solution that You outlined, so we don't have differing APIs for how |
||
``` | ||
|
||
### 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: | ||
|
@@ -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] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh interesting... I see a couple of options:
What do you think @dnamsons and @gmcgibbon? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: To Then we can remove this API and its behavior can be ported to an ERB parser extension. |
||
``` | ||
|
||
## Using the cache | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,7 +56,7 @@ def call(absolute_file) | |
private | ||
|
||
sig do | ||
params(node: Parser::AST::Node, absolute_file: String).returns(T::Array[UnresolvedReference]) | ||
params(node: ::Parser::AST::Node, absolute_file: String).returns(T::Array[UnresolvedReference]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seeing the prefixed |
||
end | ||
def references_from_ast(node, absolute_file) | ||
references = [] | ||
|
@@ -68,14 +68,14 @@ def references_from_ast(node, absolute_file) | |
references | ||
end | ||
|
||
sig { params(absolute_file: String, parser: Parsers::ParserInterface).returns(T.untyped) } | ||
sig { params(absolute_file: String, parser: Packwerk::Parser).returns(T.untyped) } | ||
def parse_into_ast(absolute_file, parser) | ||
File.open(absolute_file, "r", nil, external_encoding: Encoding::UTF_8) do |file| | ||
parser.call(io: file, file_path: absolute_file) | ||
end | ||
end | ||
|
||
sig { params(file_path: String).returns(T.nilable(Parsers::ParserInterface)) } | ||
sig { params(file_path: String).returns(T.nilable(Packwerk::Parser)) } | ||
def parser_for(file_path) | ||
@parser_factory.for_path(file_path) | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
# typed: strict | ||
# frozen_string_literal: true | ||
|
||
module Packwerk | ||
module Parser | ||
extend T::Helpers | ||
extend T::Sig | ||
|
||
requires_ancestor { Kernel } | ||
|
||
interface! | ||
|
||
@parsers = T.let([], T::Array[Class]) | ||
|
||
sig { params(base: Class).void } | ||
def self.included(base) | ||
@parsers << base | ||
end | ||
|
||
sig { returns(T::Array[Parser]) } | ||
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 | ||
|
||
sig { abstract.params(path: String).returns(T::Boolean) } | ||
def match?(path:) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,34 +9,9 @@ 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 | ||
|
||
ERB_REGEX = /\.erb\Z/ | ||
private_constant :ERB_REGEX | ||
|
||
sig { params(path: String).returns(T.nilable(ParserInterface)) } | ||
sig { params(path: String).returns(T.nilable(Packwerk::Parser)) } | ||
def for_path(path) | ||
case path | ||
when RUBY_REGEX | ||
@ruby_parser ||= Ruby.new | ||
when ERB_REGEX | ||
@erb_parser ||= erb_parser_class.new | ||
end | ||
end | ||
|
||
def erb_parser_class | ||
@erb_parser_class ||= Erb | ||
end | ||
|
||
def erb_parser_class=(klass) | ||
@erb_parser_class = klass | ||
@erb_parser = nil | ||
Packwerk::Parser.all.find { |parser| parser.match?(path: path) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if someday we'll want to allow multiple parsers to parse the same file if they do different things (e.g. a ruby file parser that takes a special DSL and turns it into a constant reference, although, like relations, I think that is currently done in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose we probably don't need to change this now since the current behavior only allows one parser for a file! |
||
end | ||
end | ||
end | ||
|
This file was deleted.
There was a problem hiding this comment.
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
andValidator
as names for the interfaces to be included to extend packwerk. Can we just call thisParser
, i.e.include Packwerk::Parser
There was a problem hiding this comment.
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 addedPackerk::
to every usage of this interface.There was a problem hiding this comment.
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
?