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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pre-commit hooks #2718

Open
Strift opened this issue Feb 19, 2024 · 6 comments
Open

Add pre-commit hooks #2718

Strift opened this issue Feb 19, 2024 · 6 comments
Labels
tooling and maintenance Maintenance (CI, tooling...)

Comments

@Strift
Copy link
Contributor

Strift commented Feb 19, 2024

It often happens that PR are submitted (by me 馃ぁ) without running the linter first.

Adding a pre-commit hook to automatically run the linter before creating a commit can help with ensure that this is done and doesn't waste reviewers time.

(It can still be possible to commit bypass this protection if needed when doing quick & dirty stuff, the CI will still run for PRs)

@Strift Strift added the tooling and maintenance Maintenance (CI, tooling...) label Feb 19, 2024
@guimachiavelli
Copy link
Member

The docs team had this discussion a long time ago and I believe @curquiza was against running linters on actions because this might dissuade contributors from submitting PRs.

@curquiza
Copy link
Member

Yeah I'm not a big fan of imposed pre-commit hook. It definitely can prevent people from submitting. It prevented me in the past for example 馃槄
We would rather have a contribution with linting issue (we can update ourselves) rather than no contribution at all. The linting problem should be visible in the CI associated to the PR. It's already the case right?

But only my opinion, I had bad experiences with pre-commit hook when trying to contribute to other opensource repositories.

@curquiza
Copy link
Member

Also, I think pre-commit hook can be set up individually (so you can set it up on your side @Strift) without imposing it to everyone

@guimachiavelli
Copy link
Member

You have infinitely more experience in managing public repositories, @curqui, so I'm inclined to side with you on this matter. I do empathize with @Strift, though, as I have uh occasionally forgotten to run linters.

@curquiza
Copy link
Member

curquiza commented Mar 25, 2024

Even if I have more experience, I don't have THE experience 馃槉
IMO, the most important is you also feel comfortable with your tool. So, the best world is to be able to customize pre-commit hook on your sides only, and don't impose it on contributors.

If not possible, I would go with imposing it on everyone if it really helps your day-to-day work. Your contributions are more than the external contributor's ones, and bring more impact and quality. It's important you feel comfortable with your work tools.

@Strift
Copy link
Contributor Author

Strift commented Mar 25, 2024

I think it's fine to have it in CI only for now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling and maintenance Maintenance (CI, tooling...)
Projects
None yet
Development

No branches or pull requests

3 participants