fluid-project / fluid-lint-all

Consolidated linting logic free from any particular build technology
BSD 3-Clause "New" or "Revised" License
0 stars 5 forks source link

Add the ability to lint only staged files #62

Closed duhrer closed 2 months ago

duhrer commented 8 months ago

Currently, fluid-lint-all runs all checks against all the files it can find. As mentioned by @greatislander in the Fluid group chats, this is slower than only linting files that have changed.

In thinking it through, I think we can fairly easily at a flag/configuration option to only lint uncommitted changes.

There is a node wrapper for git commands, but this seems excessive.

I was thinking of simply using exec and parsing the output, as we do elsewhere in this package.

git provides a machine-parseable format with its -z option:

> git status -z  
 M .eslintrc-fluid.json D fluid-config.js M package.json?? foobar%   

Each file entry is terminated with a NUL character (ASCII 0) and is separated from its status by a space operator. Since we are filtering only to the changes, we don't need to worry about deleted content (D modifier). Added (A), changed (M), and untracked files (??) should be included in the list of relative paths that have been changed.

Renamed files are slightly more complex. In theory, we could just look for the RM modifier (renamed and modified), but this would be a problem in cases where the extension (and thus linting rules) change. IMO we should include renamed (but otherwise unchanged) files (R modifier) in the scan. The format for renamed files also is slightly more complex, with three fields (modifier, old path, new path). For both R and RM, we would simply add the new path to the list of changes.

Once we have the list of relative paths to include, we prepend the repository root to the path so that we can directly compare full paths. We then derive the full list of potential files from command-line parameters and configuration files, as we usually do. Right before scanning, we filter the files to those that have changed. This allows authors to keep control over which files are checked where.

If there are no files to scan as a result of filtering for changes, there should be a clear message displayed for the relevant check.

duhrer commented 8 months ago

In refreshing my understanding of the code, I see that we already do something similar for .gitignore files:

https://github.com/fluid-project/fluid-lint-all/blob/main/src/js/check.js#L45

We could easily do another conditional filtering pass around there.

jobara commented 7 months ago

I think I'd have a preference to not include untracked files, or at least have this be configurable. Also if you want to use this with a commit hook, you probably want to be able to only include the staged changes.

duhrer commented 7 months ago

I think I'd have a preference to not include untracked files, or at least have this be configurable.

I'm not sure what you mean by untracked files? I haven't changed the .gitignore support, which works in combination with this, so you definitely can avoid having changes to those count. Also, this is completely optional and only available via an argument, it's not the default.

Also if you want to use this with a commit hook, you probably want to be able to only include the staged changes.

I don't think git status provides the ability to distinguish between "changed and staged" and "changed and not staged". Without that, I think the best I can do here is "changed".

duhrer commented 7 months ago

In working on the eslint-config-fluid pull, I confirmed that git status -z displays the same information for "modified and not staged" as it does for "modified and staged".

jobara commented 7 months ago

In working on the eslint-config-fluid pull, I confirmed that git status -z displays the same information for "modified and not staged" as it does for "modified and staged".

When you run git status -s it uses two columns when indicating the status of the file change. The first shows staged changes the latter unstaged. I'm not sure if this is lost when using -z. (see: git status short format) Another option could be to use git diff --name-status --cached -z although I'm finding that -z adds some ^@ characters around the file names. The --cached option says to only look at staged changes. The --name-status says to only output the file name and the status. (see: git diff)

jobara commented 7 months ago

I think I'd have a preference to not include untracked files, or at least have this be configurable.

I'm not sure what you mean by untracked files? I haven't changed the .gitignore support, which works in combination with this, so you definitely can avoid having changes to those count. Also, this is completely optional and only available via an argument, it's not the default.

By untracked files I mean those which haven't been added to git. So let's say you add a new file. If you run git status -s at that point it will be denoted with ??. You can use the -u flag set to no to not show untracked files.

duhrer commented 7 months ago

Thanks for explaining, and especially for explaining the flags. I have reached out to Ned to see what his original request was about. Depending on what he says, I can see breaking this out into either two separate flags or one that supports levels of some kind.

duhrer commented 7 months ago

As long as no one wants to only check unstaged changes, I'd propose two levels:

  1. Staged changes (best for precommit hooks, sounds like what you're suggesting).
  2. Both staged and unstaged changes (my interpretation of the original request).
duhrer commented 7 months ago

In discussion on the chats, it seems like everyone was looking to lint only files staged for commit, I'll update the pull to do that.

duhrer commented 7 months ago

It turns out that git status -u is really far too aggressive and does not display actual staged changes. In reading through this stackoverflow thread, I found a better approach that does appear to work.