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

Contributing pre-commit hooks #635

Closed
brucellino opened this issue Feb 21, 2024 · 5 comments
Closed

Contributing pre-commit hooks #635

brucellino opened this issue Feb 21, 2024 · 5 comments

Comments

@brucellino
Copy link
Member

Use the pre-commit framework

I have seen a few issues and PRs raised about code quality, formatting, etc., e.g. #304

I think adding a pre-commit configuration could help.

Summary of proposed changes

I usually avoid this problem by using pre-commit hooks. by the time you've pushed the code, it's too late and if the code is formatted in the pipeline, it has to make a new commit -- generally a bit messy. Better to insist on clean commits from the author in the first place. Unfortunately, the only way to really enforce this is with the pre-receive hooks, and those are only available on the enterprise plan.

But pre-commit hooks can be easily slotted into the pipeline, and they can be used to break the build if they exit != 0

The pre-commit hook in particular is https://github.com/pre-commit/mirrors-prettier

Would that be an idea you wish to explore? If so, I'd be happy to make my first PR 🙂

@gwarf
Copy link
Member

gwarf commented Feb 22, 2024

So it's about documenting how users could set those .pre-commit hooks into their local repo fork+clone?
How does it work with PR that are made by creating a branch in this upstream repo?
We also don't want to just reject any submission (like from less technical people using only the basic github web UI to make some small change), at least once we have a PR we can help getting things cleaned.

@brucellino
Copy link
Member Author

Yes, the .pre-commit-config.yml file declares which hooks are active.

We can reasonably expect a contributor to install them in their development environment:

$ pip install pre-commit
$ pre-commit install

It is not required to install them, but since they declare the baseline hygiene of the codebase, it makes it much easier to converge on. Contributors can look at them and have an idea of the conventions adopted in the codebase.

They can be included in all PR CI runs either breaking the build, or emitting a warning.

As I mentioned, running pre commit hooks after the actual commit has happened is a bit ... "wrong", so the best case scenario is that they are installed and used by the main contributors. If not, you run the risk of having to manually clean the commit history if something bad (like a secret, a huge file, or something that breaks prod) gets into the codebase by accident.

Shall I send a PR and we can have a look?

@brucellino
Copy link
Member Author

WIP #636

enolfc pushed a commit that referenced this issue Feb 26, 2024
# Summary

This adds pre-commit hooks and a few configuration files for enforcing
style guides.

Specific hooks are:

1. [prettier](https://github.com/pre-commit/mirrors-prettier) -- format
code before it's committed to respect the language standard it's written
in.
2.
[check-jsonschema](https://github.com/python-jsonschema/check-jsonschema)
-- ensure that Github Action workflows respect the schema
3. [markdown-link-check](https://github.com/tcort/markdown-link-check)
-- check links on changed markdown files

---

**Related issue :** #635 #157

---------

Signed-off-by: Bruce Becker <[email protected]>
@brucellino
Copy link
Member Author

This can be closed -- sorry forgot to mention it in #636

@brucellino
Copy link
Member Author

Closed by #636

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

No branches or pull requests

2 participants