alirezanet / Husky.Net

Git hooks made easy with Husky.Net internal task runner! 🐶 It brings the dev-dependency concept to the .NET world!
https://alirezanet.github.io/Husky.Net/
MIT License
632 stars 29 forks source link

Include parameter is ignored when no variable is used inside task #106

Closed fdipuma closed 2 weeks ago

fdipuma commented 2 months ago

Version

0.6.4

Details

I was not super sure if to open this as a bug or as a feature request, still I think the expected behavior differs to how it works now.

If I use the include parameter inside a task, but no variable (e.g. {staged}) is used in the command args, the include filter is completely ignored and the task is executed always, even if there are no changes to files matched by the glob pattern.

e.g.

{
  "tasks": [    
    {
      "name": "npx-lint-staged",
      "group": "pre-commit",
      "command": "npx",
      "args": [
        "lint",
        "staged"
      ],
      "include": [
        "client/**/*"
      ]
    }
  ]
}

If I commit a change in another folder (e.g. server/test.txt), the task npx-lint-staged is executed.

Is this the intended behavior? I think it really confuses the user right now.

Thanks!

Steps to reproduce

expected: the task created in step 1 is skipped because no file is matched actual: the task created in step 1 is executed

alirezanet commented 2 months ago

Hi @fdipuma , unfortunately, I'm a little bit busy this week but I'll look into this the next week.

fdipuma commented 1 month ago

Hey @alirezanet , thanks, we are currently using a bad workround, something like this:

{
  "tasks": [    
    {
      "name": "npx-lint-staged",
      "group": "pre-commit",
      "command": "bash",
      "args": [
        "-c",
        "test -n \"${staged:-}\" && npx lint-staged" // workaround because include does not work without ${staged} var
      ],
      "include": [
        "client/**/*"
      ]
    }
  ]
}

But this generates a lot of issues when there are too many staged files:

--------------------------------------------------
[Husky] ⚡ Preparing task 'npx-lint-staged'
[Husky] ⚠️ The Maximum argument length '8191' reached, splitting matched files into 5 chunks
[Husky] ⌛ Executing task 'npx-lint-staged' ...
Command line is too long.
  ❌ Task 'npx-lint-staged' failed in 19ms
husky - pre-commit hook exited with code 1 (error)

Do you have any other workaround? Or some idea on when you could look at the issue?

I would also like to create a PR if you like, but I'm not 100% sure where to act.

alirezanet commented 1 month ago

Hi @fdipuma, sorry for the delay in getting back to you. You're right, this is a known issue. Currently, the Include/Exclude feature only functions with variables because it's challenging to determine whether the user intends to ignore a task or execute it. (since we're not limited to pre-commit hooks) I had overlooked this problem, but I'll make it a priority to find a solution soon. As you're directly dealing with this, do you have any suggestions? Please inform me if you think any particular/potential feature could address your needs.

alirezanet commented 1 month ago

one potential fix for this would be, if we didn't have any variables in the arguments, we could consider using git staged files for the glob patterns (include/exclude) by default or with a configuration like "UseGitStagedFiles": true. do you think this works for you?

fdipuma commented 1 month ago

@alirezanet yes, I agree with you that some property is less error prone from the user perspective. But I was thinking an enum could be better than a boolean prop:

{
  "tasks": [    
    {
      "name": "npx-lint-staged",
      "group": "pre-commit",
      "command": "npx",
      "fallbackFilteringRule": "git-staged",  // use this when no param is in the args, this could be an enum and the default value could be "none" if we want to keep compatibility, and other values could be "git-staged", "git-files", "all-files", and so on
      "args": [
        "lint",
        "staged"
      ],
      "include": [
        "client/**/*"
      ]
    }
  ]
}

WDYT?

alirezanet commented 2 weeks ago

Hello @fdipuma, I've just released v0.7.0. You can view the release notes here: release notes. Could you please try this version and confirm if it resolves the issue?

You can simply add "filteringRule": "staged" to your task.

fdipuma commented 2 weeks ago

Hey @alirezanet , everything is working flawlessly now.

Thanks a lot.