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

feat: don't show when commands are skipped because of no matched files #468

Merged
merged 4 commits into from
Apr 13, 2023

Conversation

technicalpickles
Copy link
Contributor

Closes #467

⚡ Summary

If you have lots of commands, and make small commits, you'll often see a lot of skipped commands:

sorbet: (SKIP. NO FILES FOR INSPECTION)
stylelint: (SKIP. NO FILES FOR INSPECTION)
codeowners: (SKIP. NO FILES FOR INSPECTION)
authorization: (SKIP. NO FILES FOR INSPECTION)
svgo: (SKIP. NO FILES FOR INSPECTION)
syncpack: (SKIP. NO FILES FOR INSPECTION)
eslint: (SKIP. NO FILES FOR INSPECTION)
packwerk: (SKIP. NO FILES FOR INSPECTION)
rubocop: (SKIP. NO FILES FOR INSPECTION)

I wanted a way to skip this. I'm migrating from a home grown hook runner, and it had pretty concise output that I'm trying to recreate. I was able to do that with most of the other options, but still had this output.

I apologize in advance, this is my first go contribution to anything 🤪

☑️ Checklist

  • Check locally
  • Add tests

@technicalpickles
Copy link
Contributor Author

I've been looking over the repo, and found this in the troubleshooting: https://github.com/evilmartians/lefthook/wiki/Troubleshooting#skip-no-files-for-inspecting

This means that after filtering the git files, nothing remains. You can debug filtres by -v flag. Like this:

lefthook -v run pre-commit
This also happens often for fresh repositories. Just commit something.

I'm not sure if this is accurate. This is a pretty old monolith, and I see this pretty often 😅 I'm able to "fix" it by having something that matches a pattern to be committed.

Copy link
Member

@mrexox mrexox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contribution! This totally makes sense (although skip skip sounds funny). I have a small suggestion to refactor the code and put a skip settings check into a logSkip function.

I'd also suggest to name the skip_output setting skips, so it sounds a bit more natural (like, skip the skips) since we probably want to skip all kinds of skips logging that we have in lefthook 🙂

If you don't have time for these edits I can do it for you, but I'll wait for your response 🙌

internal/lefthook/runner/runner.go Outdated Show resolved Hide resolved
@technicalpickles
Copy link
Contributor Author

although skip skip sounds funny
That was kinda the appeal honestly 😂

I'd also suggest to name the skip_output setting skips,

Sure, that sounds good. I mostly did it to be consistent with the other variables. Although, unlike the others... "There are plenty places that log about skipping", so maybe that's more appropriate.

I think it's better put this code in logSkip function and make it a Runner's method in order to be able to check SkipSettings.

Can do! I considered that, but I wasn't able to see any of those locally. If you are good with it, I can make the change.

If you don't have time for these edits I can do it for you, but I'll wait for your response 🙌

I can take care of it! Thanks for the offer though.

@technicalpickles technicalpickles requested a review from mrexox April 12, 2023 16:53
Copy link
Member

@mrexox mrexox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Now everything is perfect!

@mrexox mrexox changed the title skip_skip: don't show when commands are skipped because of no matched files feat: don't show when commands are skipped because of no matched files Apr 13, 2023
@mrexox mrexox merged commit 1ade029 into evilmartians:master Apr 13, 2023
@technicalpickles technicalpickles deleted the skip-skip branch April 13, 2023 14:55
@technicalpickles
Copy link
Contributor Author

Thanks! I appreciate the feedback, merging, and release 🌈 🚀 🌟

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants