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 hooks #360

Merged
merged 23 commits into from
Feb 9, 2023
Merged

Add pre-commit hooks #360

merged 23 commits into from
Feb 9, 2023

Conversation

lobis
Copy link
Member

@lobis lobis commented Jan 9, 2023

lobis Large: 144677

The goal of this PR is to integrate with pre-commit, which in my experience is the current standard for managing pre-commit hooks.

pre-commit can be installed via pip (see documentation at the link above) and it has a very nice ecosystem of multi-language hooks which are really useful: clang-format, cmake-format, python formatting (black), etc. It also allows to define custom hooks locally which may be useful.

After installation the hooks will run after a commit only on the stage files.

I have also added a step in the CI to run pre-commit resulting in a pipeline failure if things such as the formatting of the code is not correct because a user failed to run clang-format manually or didn't use the hooks (because they were not installed or some other reason).

In the future I will also add the integration with pre-commit bot (not sure how to do this yet or how much work it could take). This will result in automatic formatting (with the bot as author) of code whenever a user fails to format the code properly in a PR.

Besides formatting, we could also add linting to the pre-commit, however this will require a very tedious revision of the code in order to pass since we haven't run any linters up to this point on the whole codebase.

In this PR I have also modified the relevant files in order to pass the formatting (python and cmake at the time of writting). These are only formatting changes. The style of formatting can be altered with configuration files.

@lobis lobis requested review from jgalan, juanangp and a team and removed request for jgalan January 9, 2023 10:08
@jgalan
Copy link
Member

jgalan commented Jan 9, 2023

I thought the code was already formatted automatically, so no manual formatting was required

@lobis
Copy link
Member Author

lobis commented Jan 9, 2023

I thought the code was already formatted automatically, so no manual formatting was required

We implemented a very rudimentary version of this utility via cmake (copy files into the hooks directory) but this relies on the user installing the required packages on its own (clang-format, xmlint, etc.). If we knew this existed we would never have done that.

pre-commit is much more powerful and easier, it uses python virtual environments so the user does not have to install anything (only pre-commit) and it allows for more powerful hooks such as and xml formatting tool that actually takes options or cmake formatting. Also the current implementation of hooks does not work for remote development and gives a cmake warning when not working.

I would remove the previous implementation in favour of this one.

@jgalan
Copy link
Member

jgalan commented Feb 9, 2023

@lobis what it is left for this to go through?

@lobis lobis marked this pull request as ready for review February 9, 2023 15:41
@lobis lobis added policy How things should behave style Related to code styling labels Feb 9, 2023
@jgalan
Copy link
Member

jgalan commented Feb 9, 2023

Large: 144677 lines!

@lobis lobis merged commit e3f8611 into master Feb 9, 2023
@jgalan jgalan deleted the lobis-precommit branch February 10, 2023 09:46
@jgalan
Copy link
Member

jgalan commented Feb 10, 2023

@lobis plz, could you add some instructions in a new page "Pushing to the repository" at https://rest-for-physics.github.io/rest-advanced with what we need to do in our repositories, the two lines we need to execute basically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
policy How things should behave style Related to code styling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants