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 hook #69

Merged
merged 13 commits into from
Nov 15, 2022
Merged

Add pre commit hook #69

merged 13 commits into from
Nov 15, 2022

Conversation

trinitronx
Copy link
Contributor

@trinitronx trinitronx commented Apr 27, 2022

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 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: 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 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 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 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
      
  • Existing issue is referenced if there is one:

  • Unit tests for the proposed change

trinitronx added a commit to LyraPhase/uncommon-build that referenced this pull request Apr 28, 2022
trinitronx added a commit to trinitronx/gospikes that referenced this pull request Apr 28, 2022
@colindean
Copy link
Contributor

This is great! Thanks for doing this up.

@trinitronx
Copy link
Contributor Author

Ping @mrtazz

@richtong
Copy link

Yes please ccept the PR :-)

@renefritze
Copy link

renefritze commented Aug 4, 2022

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?

@mrtazz mrtazz merged commit 4e37f3a into mrtazz:main Nov 15, 2022
@mrtazz
Copy link
Owner

mrtazz commented Nov 15, 2022

thanks for taking the time to contribute!

@trinitronx
Copy link
Contributor Author

trinitronx commented Jul 14, 2024

[...SNIP...] because no file is then passed to checkmake

@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 -a / --all-files flag would pass).

I noticed that you were using the "0.2.2" pre-release testing tag in my fork, which was prior to both of those changes I mentioned above. This tag in my forked repo was meant mostly for my own internal testing, as well as anyone who wanted to alpha/beta test these changes prior to my pull requests being merged upstream (here on this repo). You may have realized this by now, but if you used the 0.2.3 or later tags from my fork, it probably would have worked. However, now in 2024, everything seems to be working well using the official 0.2.2 tag on this repo (mrtazz/[email protected]).

Also, many thanks to @colindean for pushing the rest of the needed changes for Makefile .make file extension support, and the checkmake hook support upstream to this repo, pre-commit, and pre-commit/identify projects! :shipit: 📦 🚀
Coordinating changes across so many repos is a no small feat of Open Source collaboration, and is greatly appreciated! Thanks! 🙏

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 pre-commit gc, pre-commit clean, and pre-commit autoupdate for an old project, everything works well using this official repo's 0.2.2 tag!

🎉 🍾 Thanks @mrtazz!

trinitronx added a commit to LyraPhase/uncommon-build that referenced this pull request Jul 17, 2024
Merged PRs: mrtazz/checkmake#69, and mrtazz/checkmake#70

These were released in `checkmake` version 0.2.2
trinitronx added a commit to LyraPhase/uncommon-build that referenced this pull request Jul 17, 2024
Merged PRs: mrtazz/checkmake#69, and mrtazz/checkmake#70

These were released in `checkmake` version 0.2.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants