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

Add pre-commit? #180

Closed
felixhekhorn opened this issue Sep 21, 2022 · 5 comments · Fixed by #310
Closed

Add pre-commit? #180

felixhekhorn opened this issue Sep 21, 2022 · 5 comments · Fixed by #310
Assignees
Labels
enhancement New feature or request

Comments

@felixhekhorn
Copy link
Contributor

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

@felixhekhorn felixhekhorn added the enhancement New feature or request label Sep 21, 2022
@alecandido
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

cschwan commented Sep 23, 2022

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
Copy link
Member

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.

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

Successfully merging a pull request may close this issue.

3 participants