-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(alejandra): one --exclude
flag per file
#433
Conversation
@@ -1659,7 +1659,7 @@ in | |||
cmdArgs = | |||
mkCmdArgs (with hooks.alejandra.settings; [ | |||
[ check "--check" ] | |||
[ (exclude != [ ]) "--exclude ${lib.escapeShellArgs (lib.unique exclude)}" ] | |||
[ (exclude != [ ]) "${lib.escapeShellArgs (map (file: "--exclude '${file}'") (lib.unique exclude))}" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exclude
has a specific meaning in pre-commit
, which is probably not this.
Wouldn't it be better to let pre-commit pass the list of changed files anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand.
Afaik, pre-commit already passes in a list of changed files.
My use case for --exclude
with alejandra is to prevent it from formatting nix files that were generated (e.g. with a cabal2nix hook).
In any case, passing multiple files to --exclude
doesn't work with alejandra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not well documented upstream, but the pre-commit exclude
field is a regex. I now see my mistake: I confused hooks.alejandra.excludes for hooks.alejandra.settings.exclude.
So we have
- pre-commit:
exclude
, singular, regex - git-hooks.nix:
excludes
, plural, regex - alejandra settings:
exclude
, unrelated
where 1 and 2 are the same concepts, but 3 is something else.
I suppose 3 could be removed without loss of functionality, but I don't know whether keeping or removing is more harmful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose for the use case of generating nix files with cabal2nix, it would make sense to use
git-hooks.nix:
excludes
, plural, regex
I'll give it a go in a few days and report back here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just tested the git-hooks excludes, and it works fine for my use case.
So I would agree that alejandra.settings.excludes is redundant.
alejandra --exclude file1 file2 file3
doesn't work.It should be
alejandra --exclude file1 --exclude file2 --exclude file3