search

Shopify / theme-check-action

Run shopify/theme-check on GitHub pull requests
Other
38 stars 14 forks source link

Feature Request: Only lint modified files #3

Closed sergeh closed 2 years ago

sergeh commented 3 years ago

Currently, the action runs on all files, it would make more sense to only lint modified liquid files or at least have an optional argument to enable it, something along the lines of Github's Super-Linter

I'm not sure if that's possible considering there are checks like UnusedSnippet that would need to parse the entire codebase.

charlespwd commented 3 years ago

This has been brought up before. Like you said this is a hard problem because we have a bunch of checks that are project wide (unused translations and such).

In the spirit of CI however, should we really make a theme that doesn't check? An alternative approach would be to downgrade the severity of all checks to suggestion until you can bring them back to error.

Then you can gradually make PRs that fixes all the warnings to finally reset the severity to its default value.

In your .theme-check.yml file, you can do this by setting the severity property. Like this:

SyntaxError:
  enabled: true
  ignore: []
  severity: suggestion

There are more details in this section of the README.

sergeh commented 3 years ago

In the spirit of CI however, should we really make a theme that doesn't check?

While I do agree with that statement, a regular PR wouldn't normally break things that have already been linted and not modified. In our case, we only lint modified files for features, enhancements, fixes, etc. but we'll lint the entire codebase when preparing a release a PR. It saves a bit of time in terms of action minutes used.

As I said, I realize this may not even be possible and the time saved would probably be negligible on most themes.

vfonic commented 2 years ago

I regularly work in legacy themes that have tons of errors that I have no intention of ever fixing. It makes more sense to replace the theme altogether than to try to fix all of the issues in the old theme.

I think it would be valuable for end users (theme developers) to be able to choose, via some setting, whether they want a full theme check or just the check/annotations on the changed lines.

Can the tool still perform the same check like now, but just not report on the unmodified lines/files?

charlespwd commented 2 years ago

Hmmmmm. Maybe I can get a list of all the files that were diff'ed and then only push annotations for the ones that are in that list? I'll still do a full theme check but I just won't push annotations for the ones that weren't touched?

Considering the undocumented 1000 annotation limit, this might be required after all.