Shopify / erb-lint

Lint your ERB or HTML files
MIT License
593 stars 114 forks source link

Added Rake support #280

Closed bkuhlmann closed 1 year ago

bkuhlmann commented 1 year ago

Overview

The following adds Rake support so it's easier to wire up this gem using Rake and run code quality analysis from the CLI (or wired up via CI).

Details

Notes

Something is off with the way FakeFS works where the full RSpec test suite won't run with this new spec. You can only verify this works locally by running:

bundle exec rspec spec/lib/rake/task_spec.rb

I'm not familiar with FakeFS so maybe there is a configuration option that I'm missing?

etiennebarrie commented 1 year ago
require "erb_lint/rake/setup"

I find that API/entrypoint a bit weird. For comparison, here's RDoc:

require "rdoc/task"
RDoc::Task.new(:rdoc)

Bundler gem tasks:

require "bundler/gem_tasks"

Rubocop:

require "rubocop/rake_task"
RuboCop::RakeTask.new

I think it would be useful to be able to configure the task, without having to pass arguments on the command-line. Ideally you want the task to be pre-configured so that you can add it as one of the default task dependencies and be done with it.

task default: [:test, :rubocop, :erb_lint]

Also Rake's handling of arguments with the [] on the command-line is pretty peculiar, though it works, but from another task, it's pretty annoying, as they can't seem to be used as dependencies, you have to invoke them programmatically, something like:

task default: [:test, :rubocop, :my_erb_lint]
task :my_erb_lint do
  Rake::Task[:erb_lint].invoke("--config", "config/erb-lint.yml", "--lint-all")
end

Instead we should have something more similar to Rubocop where you call new (and you can choose the name of the task), then you can pass configuration to the task (I think just passing an array that acts as ARGV is fine for that).

bkuhlmann commented 1 year ago

Thanks. I've incorporated your feedback by rebasing all changes (see the README for further details). I'll respond to your notes in the order defined:

:warning: There is still an issue with running the full test suite in regards to FakeFS. For some reason spying on the CLI seems to cause havoc.

rafaelfranca commented 1 year ago

I think the same position applies here https://github.com/Shopify/tapioca/issues/1155#issuecomment-1244891571.

I'm not sure what value this provides. If all you want is to hook the erb-lint CLI in your Rake tasks, why can't you do

task :erb_lint do
  `erb-lint`
end

in your Rakefile?

Do we really need to maintain two CLI interfaces in this library?

bkuhlmann commented 1 year ago

Hey Rafael, thanks. Make sense. I'll close this down in favor of inllining the task. :bow: