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

Touch up README.md and run clang-format via precommit hooks #752

Merged
merged 2 commits into from
Feb 17, 2024

Conversation

MatthewDaggitt
Copy link
Collaborator

This touches up the docs and adds instructions to them on how to get clang-format installed as a pre-commit hook in a slightly more robust way. In particular, we use pre-commit to ensure there's no need for a special bash script any more, and to make it easy for everyone to ensure that formatting runs whenever they commit.

@wu-haoze if you run then everything will be setup for you.

pip install pre-commit
pre-commit install

@wu-haoze
Copy link
Collaborator

@MatthewDaggitt Thanks for making this change! I suggest we keep the bash script as a low commitment option for one-time developers.

@MatthewDaggitt
Copy link
Collaborator Author

I suggest we keep the bash script as a low commitment option for one-time developers.

The problem is the bash script doesn't actually work for me.... In particular the shell I use doesn't calculate the path correctly with SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )".

I would argue that running the following once:

pip install pre-commit
pre-commit install

is less of a burden for one-time developers than running

apt install clang-format

and then much later, separately and repeatedly remembering to run

./tools/clang-format

(if the latter even works for them).

In particular I'm not sure we should be targeting "one-time" developers. We should be encouraging people to be at least "two-time" or "three-time" developers at least 😉

Having said that, if you would like to keep the bash script around then we can?

@wu-haoze wu-haoze merged commit c7f150d into master Feb 17, 2024
11 checks passed
wu-haoze pushed a commit that referenced this pull request Mar 4, 2024
* Touch up README.md and run `clang-format` via precommit hooks

* Python versions fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants