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

Git pre-commit: Add barebones config file #125

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

netravnen
Copy link
Contributor

@netravnen netravnen commented Oct 18, 2023

@netravnen
Copy link
Contributor Author

Git pre-commit add the possibility to introduce project specific pre-commit checks out of the box locally on the endhost of the developer/user committing changes to their own fork of the repository. This has the benefit of some standard checks having been before a PR is ever opened.

@netravnen
Copy link
Contributor Author

@pierky do you think this suggestion is worth adding?

@pierky
Copy link
Owner

pierky commented Oct 21, 2023

Hi @netravnen, I like the idea, however after I tested it I see that a lot of files would be modified because of several "inconsistencies". Most of the files identified by the precommit hooks are those which are generate automatically via Jinja2 templates, where having a wrong trailing space is pretty common (at least given the way they are built today). Also some YAML files which are purposely wrong are caught by the hooks. If we want to keep the linters are you suggested, we would need to add some exlude patterns probably, at least to ignore those files. Otherwise, every time new files are automatically built, the precommit hooks would re-format them, causing additional "noise" in the commit history. Would you be able to narrow down the list of impacted files and exclude all those which are automatically generated, maybe by keeping only "core" files in scope (like Python files and a few others)?

@netravnen
Copy link
Contributor Author

netravnen commented Oct 21, 2023

Narrowing the scope to core files would be a good start. Causing the least friction.

Will update the branch with this suggestion.

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

Successfully merging this pull request may close these issues.

2 participants