-
Notifications
You must be signed in to change notification settings - Fork 45
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 hook #69
Conversation
…to parent repo This reverts commit a71eb88.
…/checkmake#70 are merged upstream
…azz/checkmake#70 are merged upstream
This is great! Thanks for doing this up. |
Ping @mrtazz |
Yes please ccept the PR :-) |
It seems the hook will currently not work as documented when run from
then that mode also works. Maybe this is something you'd want to document or maybe there's a smart way to handle this on the hook side? |
thanks for taking the time to contribute! |
@renefritze: Sorry for the belated reply (too many GitHub notifications were cluttering my inbox for years). Yes, this was fixed in a12e9a0 and 2d14c7a which also slightly depended on #70 being merged as well for multiple files support (which the I noticed that you were using the " Also, many thanks to @colindean for pushing the rest of the needed changes for For more details, see:
I've revisited using this hook again in 2024, and it appears that all the necessary bits across all these projects are now merged and released! 🚀 After a 🎉 🍾 Thanks @mrtazz! |
Merged PRs: mrtazz/checkmake#69, and mrtazz/checkmake#70 These were released in `checkmake` version 0.2.2
Merged PRs: mrtazz/checkmake#69, and mrtazz/checkmake#70 These were released in `checkmake` version 0.2.2
Checklist
Not all of these might apply to your change but the more you are able to check
the easier it will be to get your contribution merged.
CI passes
See the included GitHub Actions workflow on my fork here. It is expected to fail on this repo until tag
0.2.2
exists (see notes below).Description of proposed change:
Makes this repo a fully-fledged
pre-commit
hook.Now that Go modules are integrated into new versions of GoLang, the simplest path
forward for getting this
pre-commit
hook working again is to include it in this repo.pre-commit
hooks withlanguage: golang
are designed to be included along with the gomodule's github repo project.
Problem that was introduced by new GoLang versions: Unable to use checkmake hook Lucas-C/pre-commit-hooks-go#2
As noted in Integration as a pre-commit hook #19,
go get ./...
, and nowgo install ./...
commandsare based on certain assumptions. With newer versions of GoLang and now
go modules, there are further assumptions that
go.mod
andpackage
strings matchthe repo name, and this does not work for a wrapper repo that tries to include this one
because it uses
package main
.Thus, the original pre-commit hook in Integration as a pre-commit hook #19 is now broken. I tried a few hack-ish methods of
wrapping or including this repo as a go module under
additional_dependencies
.However, those methods were messy and did not work.
Documentation (README, docs/, man pages) is updated
NOTE: This depends on an immutable git tag or sha to be committed to this repo, because
pre-commit
requires anyrev
specified to be immutable.The included
.pre-commit-config.yaml
specifies0.2.2
, under the assumption thatthis tag will exist after this Pull Request is merged.
Existing issue is referenced if there is one:
Unit tests for the proposed change
NOTE: As noted above, this depends on an immutable git tag
0.2.2
being created onthis repo after this Pull Request is merged.
I've run the included GitHub Actions workflow on my fork here, by creating a fork-only local tag:
0.2.2
. It's passing there, but is expected to fail here until that tag exists.EDIT: Additionally, I've pushed up another PR Feature/add support for multiple makefiles #70, which fixes the use case for running against multiple
Makefile
/*.mk
/*.make
file patterns. I've enabled this feature in commits 2d14c7a and a12e9a0.Without merging Feature/add support for multiple makefiles #70, the
pre-commit
run will return nonzero exit status and print the usage text forcheckmake
. This is because previous to Feature/add support for multiple makefiles #70, it did not support passing multiple filenames on the command line.