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

Exclude option not working if no staged_files use #441

Closed
ctjhoa opened this issue Mar 3, 2023 · 3 comments
Closed

Exclude option not working if no staged_files use #441

ctjhoa opened this issue Mar 3, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@ctjhoa
Copy link

ctjhoa commented Mar 3, 2023

🔧 Summary

I want to exclude files from a lefthook command but I do not use files variable in my run command line.

Lefthook version

1.2.7 c88a009cfa095e97ce47f8c5d5e1db5e6c844341

Steps to reproduce

# lefthook.yml
pre-commit:
  parallel: true
  commands:
    lint:
      tags: lint projects
      exclude: 'apps/memory-360.*'
      glob: '*.{ts,tsx,js,jsx,rb,ru}'
      run: yarn affected:lint --uncommitted

Expected results

run should not be executed when an excluded file is affected.

Actual results

run is always executed.

Possible Solution

I found a workaround by turning my lefthook config into this

# lefthook.yml
pre-commit:
  parallel: true
  commands:
    lint:
      tags: lint projects
      exclude: 'apps/memory-360.*'
      glob: '*.{ts,tsx,js,jsx,rb,ru}'
      run: yarn affected:lint --uncommitted && echo {staged_files} > /dev/null

Screenshots (if appropriate)

Actual results
2023-03-03-170524_1278x113_scrot

With workaround

2023-03-03-170501_1269x335_scrot

@ctjhoa ctjhoa added the bug Something isn't working label Mar 3, 2023
@mrexox
Copy link
Member

mrexox commented Mar 6, 2023

Hi! This issue totally makes sense, but things are a bit complicated.

You have pre-commit hook and a command that doesn't reference any files template and doesn't provide a files option. It is fine but when you have glob and exclude lefthook should decide: what files should the filters be applied to? for pre-commit it is {staged_files}, For pre-push it is {push_files}, and what is it for other hooks? Not so easy to answer. We've got a lot of them.

That was the reason this feature wasn't implemented. But I think it makes sense to have it with these requirements:

  • for pre-commit filter {staged_files}
  • for pre-push filter {push_files}
  • for all the others - leave the current behavior (no filtering)

Does it sound good?

The only thing I have concerns about - this approach is not verbose because it is implicit and the logic is not obvious. If you have a better suggestion I can consider it and add this feature to the next release. I don't want to bring a breaking change and unexpected behavior. I also don't want to bring non-intuitive features that could be redesigned in the future when people get used to them (like the way skip option is designed which I'm afraid to touch/change). How do you think this could be configured the best way? Does using of implicit templates feel intuitive enough?

@ctjhoa
Copy link
Author

ctjhoa commented Mar 6, 2023

I created this bug ticket for the sake of:

  • Confirming the issue
  • Avoiding debugging time waste for other people
  • Giving workaround

If the existing behavior make sense from a technical point of view / project maintenance and lefthook users have workaround, this limitation can be documented as is.
The real issue here is that it tooks me 2 hours to find out what's going on :P

@mrexox
Copy link
Member

mrexox commented Mar 16, 2023

I've released this feature in v1.3.6. Please, check it out 🚀

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

2 participants