fix: Use extensions input to filter files passed to ESLint #36
Conversation
README.md
Outdated
| @@ -31,6 +31,7 @@ jobs: | |||
| | `failure-level` | The lowest annotation level to fail on ("warning or "error"") | no | `"error"` | | |||
| | `conclusion-level` | Action conclusion ("success" or "failure") if annotations of the failure-level were created. | no | `"success"` | | |||
| | `working-directory` | Which directory to run the action in | no | `"."` | | |||
| | `extensions` | Space separated list of extenions. Only changed files with these extensions will be passed to ESlint. | no | `".js .ts .jsx .tsx .mjs .cjs"` | | |||
There was a problem hiding this comment.
Did you consider a regex pattern for this config? Any particular preference on that vs space separation?
If we allowed regex, we might be able to put something like this. It's definitely not clearer, or simpler, but I wonder if the power would ever be valuable?
.(m|c)?(j|t)sx?
There was a problem hiding this comment.
I did consider it, but I preferred the easier to read space separated version over that. Definitely could be missing something, but I can't immediately think of a solid reason you would want regex over a simple list of extensions.
There might be a very small speed improvement with using that regex, but it would come at the cost of being able to quickly read a list and know if your file should be included in the linter. Open to alternatives if you feel strongly though!
There was a problem hiding this comment.
At the risk of bike shedding here, can I suggest we go a different route: comma-separated list of extensions? I agree that simple extensions feel easier to use and reason about from both sides (using this action & building/maintaining it). Since we previously had extensions as a comma separated list, this should make the upgrade even easier without any meaningful difference for us.
There was a problem hiding this comment.
That's a good point about maintaining compatibility. I like the readability better of space separated, but that change doesn't feel worth the breaking of compatibility I think.
src/index.ts
Outdated
| let extensions = core.getInput("extensions").split(" ") | ||
| core.debug(`Extensions: ${extensions}`) | ||
| let changedFilesMatchingExtensions = changedFiles.filter((file) => | ||
| extensions.some((ext) => file.endsWith(ext)), | ||
| ) | ||
| core.debug( | ||
| `Changed files matching extensions: ${changedFilesMatchingExtensions}`, | ||
| ) |
There was a problem hiding this comment.
I'm going back and forth a bit on the manual filtering of files. For older versions of ESLint, we need to support something, but the translation of these extensions to the --ext CLI option seems a straightforward path there. If they're on a newer version of ESLint that supports the files config, can we push teams in the direction of using that in their own app?
Other than it requiring a bit of work, are there any negative consequences for the teams? It seems like we'd be pushing them in a more sustainable future for themselves too (i.e. any ESLint run in any other environment will be constrained just the same as their Balto ESLint runs are).
There was a problem hiding this comment.
Peanut gallery, but FWIW, if this is a problem that can be solved at the eslint.config.js level, I'm all for that.
There was a problem hiding this comment.
@molawson, thanks for bringing this up. I felt a bit uneasy too when working on it, but couldn't immediately think of a better solution. After looking in more to follow up on your comments I realized: this is a problem that I forgot that I already solved 😆
ESLint 9 will let you throw whatever junk you want in the file args and will filter it based off of the files option in the eslint.config.js file and the built-in defaults:
As long as you pass —no-warn-ignore it won't even complain that there are files that don't match.
I added that flag earlier this year when spiking out the "clean sheet" version for v9. A few weeks ago I pulled that flag out to maximize support for earlier versions, but now it's clear we need to add forking logic for below and above v9.
There was a problem hiding this comment.
I just got off a call with @molawson. I looked into doing the ext/files config split today and got far with it, but ran into issues with older versions of ESLint not following the ext flag the way we thought it would. Since we previously manually filtered files in v0, we decided it would be good to move forward with this current implementation (after making the space -> comma separated list change).
Because the current file is blank, it was not causing the parsing issues we are seeing with v1.0.0 and non-js files
Without this manual filtering, ESLint will attempt to lint files that should be ignored (like ruby files). ESLint v9 has the `files` option in the flat config, and earlier versions have the `--ext` CLI flag. Let's aim for maximum compatibility and just ignore the files that don't end in the extensions we want.
Summary
When a PR contains a changed ruby file, v1.0.0 of balto-eslint may annotate the ruby file with (something along the lines of)
Parsing error.This is because we currently pass all changed files to ESLint. In this PR I'm proposing we add an optional
extensionsinput so that we can pre-filter the changed files before passing them along. This is a wide list to ensure maximum compatibility before requiring an override of the default value.History
When working on the v1 rewrite, I aimed to simplify things as much as possible, so I just transparently passed in the list of all files changed. I thought ESLint would safely ignore other files passed in, but it looks like that was not entirely correct (it ignores some?).
Alternatives Considered
ESLint v9 using the "flat" config (
eslint.config.js) has thefilesoption where you can pass in an array of globs to filter out files when linting. I chose not to go with this because:ESLint v8 and lower have the
--extCLI flag. We could pass in a list of extensions there without having to do the filtering ourselves. But: