Add pre-commit? - Githubissues

NNPDF / pineappl

PineAPPL is not an extension of APPLgrid

https://nnpdf.github.io/pineappl/

GNU General Public License v3.0

13 stars 3 forks source link

Add pre-commit? #180

Closed felixhekhorn closed 1 week ago

felixhekhorn commented 2 years ago

How about activating pre-commit here? e.g. this repo provides fmt, check and clippy

alecandido commented 2 years ago

That repo is not highly reliable: not updated since 2 years.

If it's working is fine, by we could have to maintain the hooks ourselves, at some point. I'd say that either we find something more maintained, or we directly fork the repository. Such that we are free to modify it, if needed.

felixhekhorn commented 2 years ago

I think we would only need to update if cargo would change, which is highly unlikely - in any case the hooks are very simple so we could even fork and maintain them ...

alecandido commented 2 years ago

I think we would only need to update if cargo would change, which is highly unlikely - in any case the hooks are very simple so we could even fork and maintain them ...

That's the thing. In any case, I believe we're going to do it soonish, that way is also easier to add more/customize.

cschwan commented 2 years ago

I agree that using a git hook is a good idea. Right now we only write what people should do in CONTRIBUTING.md, with hooks we could automate/enforce it. However, it seems that pre-commit is mostly tailored towards Python and probably a bit too much for what we need; let's talk about that first.

Run cargo fmt --check to see if everything is properly formatted.
We probably don't want to run cargo clippy now, as this returns a very long list of warnings. At some point, however, we might consider doing that.
Do we want to run cargo check? If yes, do we want to run it with --features=applgrid,fktable,fastnlo or a subset of these (which would require having these libraries installed)?
I don't think we want to run cargo test, which is a job for our CI.

An alternative to pre-commit is writing the hooks manually, putting them into .githooks/ and activating them with git config core.hooksPath .githooks/.

alecandido commented 2 years ago

You are definitely right that pre-commit is coming from Python ecosystem, but in a sense is only the driver. Could have been written in shell or perl, and it was the same.

The main argument in favor of pre-commit is that simplifies a set of operations, and provide useful hooks out of the box. The main con is that it is definitely "non-minimal".

The minimal solution would be .githooks/, but we are adding too much to it, at some we'll start rewriting pre-commit. At that point the minimality would be defeated by duplication.