search

oppiliappan / statix

lints and suggestions for the nix programming language
https://git.peppe.rs/languages/statix/about
MIT License
552 stars 21 forks source link

extend to accept multiple files to check #62

Closed Mic92 closed 1 year ago

Mic92 commented 1 year ago

This is useful for integration into tools like treefmt (https://github.com/numtide/treefmt).

Currently we only consider the first arguments for gitignore, which should work if people run on this tool on their repository. If multiple arguments are provided they probably want to be any of these targets to be checked.

oppiliappan commented 1 year ago

thanks for the pr, running on all paths supplied makes perfect sense, and i like how unix-y it is (cat or ls would do the same). i would like to brainstorm a better way to do this however:

Currently we only consider the first arguments for gitignore, which should work if people run on this tool on their repository. If multiple arguments are provided they probably want to be any of these targets to be checked

i haven't looked at the code yet, but am i right in understanding that an invocation like so:

statix check repoA/ repoB/ repoC/

will apply ignore rules only to repoA? could we benefit from using a drop-in solution like walkdir instead?

Mic92 commented 1 year ago

It will only consider ignores from repo repoA into account. The implementation would become quite complex for a usecase I argue is not very common (most of the the time when dealing with multiple files they will from the same repository). Basically one would need to detect different repositories and assign file paths from the same repository to the same ignore set.

Mic92 commented 1 year ago

The alternative would be if multiple targets are given to not look at gitignores at all because the user already made a choice on selecting these specifically.

oppiliappan commented 1 year ago

It will only consider ignores from repo repoA into account.

understood. i just had a peek at the code too. this might cause some strange behaviour, consider this situation:

statix check repoA/ repoB/ repoC/

if we assume repoA contains a .gitignore file, then it will be used to build the ignore set. this ignore set is subsequently used to filter files from repoA, repoB and repoC. so if repoA/.gitignore contains the pattern flake.*, then repoB/flake.nix would get ignored.

you do mention that the implementation would become a little too complex, in order to support the additional ignore rules, i would be happy to continue hacking on this PR in that case.

Mic92 commented 1 year ago

you do mention that the implementation would become a little too complex, in order to support the additional ignore rules, i would be happy to continue hacking on this PR in that case.

yeah, go ahead.

Mic92 commented 1 year ago

I am going to close this since it now has a merge conflict and it's unclear what I need to do to get this merged. Without this feature statix is too slow for the projects I want to use it on.

oppiliappan commented 1 year ago

I would be happy to add this especially if it makes your workflow faster. I would prefer to continue working on this PR if and when I get the time. Would you mind creating an issue to track this?