Skip to content
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

Csharpier pre commit hook with custom variable not included in commit #120

Open
VinceBroAmilia opened this issue Sep 16, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@VinceBroAmilia
Copy link

VinceBroAmilia commented Sep 16, 2024

Version

Seen on v0.6.4 to v0.7.1

Details

So I am trying to run csharpier as a pre commit hook and support formatting files which have been renamed in git, I opted for using a custom variable that was inspired by the ${staged} variable in the husky code.

{
  "variables": [
    {
      "name": "staged-diff-files",
      "command": "git",
      "args": ["diff", "--cached", "--name-only", "--no-ext-diff", "--diff-filter=ACMRTUXB"]
    }
  ],
  "tasks": [
    {
      "name": "Run csharpier",
      "group": "pre-commit",
      "command": "dotnet",
      "args": [ "csharpier", "${staged-diff-files}" ],
      "include": [ "**/*.cs" ]
    },
  ]
}

I noticed that the formatting changes only appear post commit, unstaged.

When I use this instead :

{
  "tasks": [
    {
      "name": "Run csharpier",
      "group": "pre-commit",
      "command": "dotnet",
      "args": [ "csharpier", "${staged}" ],
      "include": [ "**/*.cs" ]
    },
  ]
}

It adds the formatting to the commit as is expected with a pre commit hook.

Even when I set the variable to the exact same as in the code for the ${staged} variable using --diff-filter=AM it does not have the same behavior, it does not add the formatting changes to the commit.

Wondering if this is expected behavior with custom variables or if this is a bug.

Steps to reproduce

Set task runner json with a variable in the csharpier task, with husky running it a pre-commit hook :

{
  "variables": [
    {
      "name": "staged-diff-files",
      "command": "git",
      "args": ["diff", "--cached", "--name-only", "--no-ext-diff", "--diff-filter=AM"]
    }
  ],
  "tasks": [
    {
      "name": "Run csharpier",
      "group": "pre-commit",
      "command": "dotnet",
      "args": [ "csharpier", "${staged-diff-files}" ],
      "include": [ "**/*.cs" ]
    },
  ]
}

Take a .cs file, change the formatting so it triggers csharpier, add it, commit it. The formatting will not be included in the commit, but will be an unstaged change after the commit, not the case when using ${staged}

@VinceBroAmilia VinceBroAmilia added the bug Something isn't working label Sep 16, 2024
@alirezanet
Copy link
Owner

Hello @VinceBroAmilia,
I will investigate this as soon as I have some free time. Currently, my schedule is quite full. I believe this issue might be addressed by Husky, so I am treating it as a bug for the time being. Thank you for your feedback.

@VinceBroAmilia
Copy link
Author

Thank you @alirezanet

@mithileshz
Copy link

Hi @alirezanet

I had a look at this one, and I think the issue seems to be because the argument type is a custom variable which creates an instance of a ExecutableTask instead of StagedTask in the ExecutableTaskFactory.cs The StagedTask does a re-staging of the files however the ExecutableTask does not.

I would love to help and contribute to this, if I could get some pointers on what a good approach to fix this could be?

Thanks

@alirezanet
Copy link
Owner

Hi @mithileshz,

Thanks for pointing this out! I think you're right—custom variables aren’t directly tied to tasks, so they don’t inherently trigger re-staging, and in most cases, it’s not needed.

One option I’m considering is adding a setting to custom variables that could toggle behavior like re-staging, but I’m not sure if that’s the best approach yet. To be honest, the exact use case for this custom variable isn’t fully clear to me. Understanding the need behind it would help explore whether a new built-in variable might be a better solution 🤔 or if we should adjust the current built-in ${staged} variable if it’s not working as expected.

For now, this might require some experimentation to figure out the cleanest way forward. Please feel free to jump in if you can help with this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants