checkmake icon indicating copy to clipboard operation
checkmake copied to clipboard

Add pre commit hook

Open trinitronx opened this issue 3 years ago • 4 comments

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.

  • [x] 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).

  • [x] 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 with language: golang are designed to be included along with the go module's github repo project.

    Problem that was introduced by new GoLang versions: Lucas-C/pre-commit-hooks-go#2 As noted in #19, go get ./..., and now go install ./... commands are based on certain assumptions. With newer versions of GoLang and now go modules, there are further assumptions that go.mod and package strings match the 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 #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.

  • [x] 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 any rev specified to be immutable. The included .pre-commit-config.yaml specifies 0.2.2, under the assumption that this tag will exist after this Pull Request is merged.

      repos:
      -   repo: https://github.com/mrtazz/checkmake.git
          rev: 0.2.2
          hooks:
          - id: checkmake
      
  • [x] Existing issue is referenced if there is one:

    • Lucas-C/pre-commit-hooks-go#2
  • [x] Unit tests for the proposed change

    • NOTE: As noted above, this depends on an immutable git tag 0.2.2 being created on this 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 #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 #70, the pre-commit run will return nonzero exit status and print the usage text for checkmake. This is because previous to #70, it did not support passing multiple filenames on the command line.

trinitronx avatar Apr 27 '22 23:04 trinitronx

This is great! Thanks for doing this up.

colindean avatar May 05 '22 01:05 colindean

Ping @mrtazz

trinitronx avatar Jun 09 '22 22:06 trinitronx

Yes please ccept the PR :-)

richtong avatar Jun 15 '22 07:06 richtong

It seems the hook will currently not work as documented when run from pre-commit run -a because no file is then passed to checkmake. If I edit the config to

-   repo: https://github.com/trinitronx/checkmake/
    rev: 0.2.2
    hooks:
    -   id: checkmake
        files: Makefile

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?

renefritze avatar Aug 04 '22 08:08 renefritze

thanks for taking the time to contribute!

mrtazz avatar Nov 15 '22 08:11 mrtazz