Skip to content
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

Support providing a custom ERB parser class #67

Merged
merged 3 commits into from
Nov 24, 2020

Conversation

joshaber
Copy link
Contributor

What are you trying to accomplish?

Fixes #66

This allows users to specify a custom ERB parser to support non-standard tags. For example, we can use it to comment out <%graphql> tags from https://github.com/github/graphql-client so that Packwerk can parse the rest of the file:

class CustomParser < Packwerk::Parsers::Erb
  def parse_buffer(buffer, file_path:)
    preprocessed_source = buffer.source

    # Comment out <%graphql ... %> tags. They won't contain any object
    # references anyways.
    preprocessed_source = preprocessed_source.gsub(/<%graphql/, "<%#")

    preprocessed_buffer = Parser::Source::Buffer.new(file_path)
    preprocessed_buffer.source = preprocessed_source
    super(preprocessed_buffer, file_path: file_path)
  end
end

Packwerk::Parsers::Factory.instance.erb_parser_class = CustomParser

What approach did you choose and why?

This is similar to how graphql-client handles this same problem for Erubi itself: https://github.com/github/graphql-client/blob/master/lib/graphql/client/erubi_enhancer.rb

What should reviewers focus on?

Is this the appropriate way to set a global configuration value? Should it be set in Configuration instead?

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)

Users can now specify a custom ERB parser:

Packwerk::Parsers::Factory.instance.erb_parser_class = YourCustomParserClass

Checklist

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

@joshaber joshaber requested a review from a team as a code owner November 11, 2020 20:05
@ghost ghost added the cla-needed label Nov 11, 2020
parse_buffer(buffer, file_path: file_path)
end

def parse_buffer(buffer, file_path:)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I split this to allow subclasses to implement #parse_buffer without concerning themselves with the actual file read.

@joshaber
Copy link
Contributor Author

CLA signed 👍

@mclark
Copy link
Contributor

mclark commented Nov 17, 2020

👋 Hi folks, is there anything we (@joshaber and I) can do to help move this along? Thanks! 🙇

@exterm
Copy link
Contributor

exterm commented Nov 19, 2020

Sorry folks, we've been a little busy with some internal stuff and the holiday season coming up. We'll get someone to look into this.

@ghost ghost removed the cla-needed label Nov 23, 2020
@exterm
Copy link
Contributor

exterm commented Nov 23, 2020

As previously the main branch was broken, could you rebase onto the current, fixed one?

Copy link
Contributor

@exterm exterm left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@exterm
Copy link
Contributor

exterm commented Nov 23, 2020

@tomstuart would you be able to give this a second review?

Copy link
Contributor

@tomstuart tomstuart left a comment

Choose a reason for hiding this comment

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

This looks clean, simple & correct to me. Thank you @joshaber!

@wildmaples wildmaples merged commit 18a3054 into Shopify:main Nov 24, 2020
@joshaber
Copy link
Contributor Author

Thanks all! ✨

@joshaber joshaber deleted the custom-erb-parser-2 branch November 24, 2020 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support customizing the ERB implementation
5 participants