UBC-Thunderbots
Git hook to prevent unformatted code from being committed
Description
This PR adds a Git pre-commit hook that tracks the last time the formatter was run, and cancels the commit and informs the user if any staged code or documentation files are more recent than the last time of format. This ensures that code style guidelines are being followed before files are committed.
Existing developers need to run git config core.hooksPath scripts/githooks from repo root; new developers will have this done automatically by setup_software.sh.
Resolved Issues
Resolves #3173
Review Checklist
(No edits to actual code)
It is the reviewers responsibility to also make sure every item here has been covered
- [x] Function & Class comments: All function definitions (usually in the
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class. - [x] Remove all commented out code
- [x] Remove extra print statements: for example, those just used for testing
- [x] Resolve all TODO's: All
TODO(or similar) statements should either be completed or associated with a github issue
Did the software tests fail because of the erroneous removal of the requirements_lock.txt by the auto fix? I'm not familiar with how the CI works yet
Did the software tests fail because of the erroneous removal of the
requirements_lock.txtby the auto fix? I'm not familiar with how the CI works yet
Looks like it, I'm not sure why it did that. Try regenerating the file by running scripts/compile_pip_requirements.sh
With pre-commit, would it be possible to preserve the behavior of just stopping a commit and notifying the developer instead of automatically running the whole formatter? I used this this approach because of how long the formatting script takes, which might be annoying when running git commit.
Considering ergonomics, maybe it might be better to have this as a pre-push hook instead? This way developers can work on all their commits and then just format once before pushing instead of at every commit, which would be a lot more frictionless.
I personally prefer the second option, but I'm willing to work on either of these. Please let me know what you think.
I see your point now about why we might not want to automatically run the formatting script, and instead just give a warning to the user. Actually, it reminds me of a discussion I had with previous leads about whether we should even be installing git hooks locally. Especially during competition when we're making a lot of changes and don't have a lot of time, I can see this pre-commit/push hook becoming very annoying if it keeps nagging at us to run the formatter. That's sort of why we opted to use pre-commit CI (to ensure that code merged into master is properly formatted and linted, without being a huge inconvenience).
Also, the issue of wasting CI time mentioned in #3173 is not really relevant because afaik github actions is free without limits for public repos.
Considering all this, I think we should use your git hook and change it so that it
- is pre-push instead of pre-commit, and
- prompts the user y/n on whether to continue with the push anyway, even if code is unformatted
EDIT: I've solved the major issues with my pre-push implementation, but there's still one issue:
If you commit changes that are considered unformatted, then do git reset --soft HEAD^ to "uncommit", even if the previous state was fine, the push hook complains about it being unformatted. I'm not quite sure how to solve this, but I don't think it's a big deal because you can tell it to push anyways.
Is there some way to commit forcefully regardless of formatting (e.g. via some flag)? It is not uncommon that, during mid-prototyping, a developer would want to be able to commit their work either to save or share their work via remote. It is also common that merge errors may arise, and some nasty commits may have to be made to prevent lost work.
Yes, the current version only warns you on commit (doesn't stop you completely), doing the formatting check on push only. You can still bypass it anyways at a y/n prompt
Quick note, once we have mac support from #3496, the stat command has slightly different syntax on mac. So it would be stat -f %m instead of stat -c %Y in the pre-commit script. Might need to have either a different script for mac or just a condition that checks which os.
Either way not that relevant right now though
