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

Fix pre push hook #1869

Merged
merged 2 commits into from
Feb 6, 2025
Merged

Fix pre push hook #1869

merged 2 commits into from
Feb 6, 2025

Conversation

fnando
Copy link
Member

@fnando fnando commented Feb 5, 2025

What

#[allow(clippy::too_many_arguments)] was being ignored, so let's use a module rule instead.

Why

So pre-commit hook works as expected.

Known limitations

N/A

@leighmcculloch
Copy link
Member

leighmcculloch commented Feb 6, 2025

Do folks use the pre-push hook? I don't use it, because I have my gitconfig configured to disable / prevent / disallow any hooks from executing, because personally I don't want commands auto-executing and I prefer to have CI alert me to issues.

@fnando
Copy link
Member Author

fnando commented Feb 6, 2025

Good question. I did enable them, but I'm not sure other people have. Should we remove it entirely?

@willemneal
Copy link
Member

I use it. Personally have the opposite opinion as @leighmcculloch. I only do the push, so that I get one check before wasting CI time on something that is trivial and usually it's the generated docs or cargo fmt which I can then amend to the last commit. And using --no-verify you can skip it if you know it's not needed.

@fnando fnando force-pushed the fix-pre-push-hook branch from fe72308 to 9e5dc50 Compare February 6, 2025 17:35
@fnando fnando enabled auto-merge (squash) February 6, 2025 17:37
@fnando fnando merged commit 1afcf24 into main Feb 6, 2025
30 of 31 checks passed
@fnando fnando deleted the fix-pre-push-hook branch February 6, 2025 18:00
@leighmcculloch
Copy link
Member

And using --no-verify you can skip it

Can also skip it with:

git config set --global core.hooksPath no-hooks

@leighmcculloch
Copy link
Member

Should we remove it entirely?

Not if folks are using it. I was just wanting to check if anyone was.

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

Successfully merging this pull request may close these issues.

3 participants