Add pre-commit hook

[amitds1997]

Fixes #539

Rust-based pre-commit does not work because pre-commit hardcodes the --path to . which leads to build failure and errors with found a virtual manifest at <path>/Cargo.toml instead of a package manifest. Pre-commit maintainer disagrees with making this configurable so it is not possible to add a rust based hook: pre-commit/pre-commit#2931

[amitds1997]

Rust-based pre-commit does not work because pre-commit hardcodes the --path to . (source) which leads to build failure with error: found a virtual manifest at <path>/Cargo.toml instead of a package manifest.

Pre-commit maintainer disagrees with making --path configurable so it is not possible to add a rust based hook: pre-commit/pre-commit#2931 unless we somehow make it possible to install the binary from root level.

[chriscerie]

I think it's possible to point entry to a custom install script. But we'd need to handle the caching, unless we distribute the executable directly.

[amitds1997]

I was unsure if we wanted a custom install script just for the pre-commit use case. I can add it if that's something we are okay with.

[chriscerie]

I think it would depend on how it would look like. Caching binaries by version may not be trivial and we don't get the same advantages language: rust gets, which makes me think it's likely not worth the effort to support this.

[amitds1997]

Agree. That's why went ahead with only setting up system and docker workflows. Is there some trivial way to make this work by may be a configuration change at our end? I did some preliminary research but did not find much (but then I have not worked with rust ever before).

[jenstroeger]

Have you looked at how StyLua handles the pre-commit hook?

[amitds1997]

Their rust workflow works because they do not use workspaces.

The python workflow seems interesting. We can integrate it, but it would depend on a python package (https://pypi.org/project/release-gitter/) to do the release download for us. If that is something we are okay with, I can add it 👍

[amitds1997]

Added GitHub release based pre-commit hook in 5f1a755

[chriscerie]

Need changelog

[chriscerie]

I think it's possible to point entry to a custom install script. But we'd need to handle the caching, unless we distribute the executable directly.

[chriscerie]

It looks like pre-commit builds docker with docker build ., which causes the light build to be used. This isn't compatible with roblox projects. It also doesn't seem like pre-commit supports changing the build command, so a rather hacky workaround could be to move the selene-musl build to the last in the dockerfile.

[amitds1997]

@chriscerie @jenstroeger Let me know if there's something else needed from my end to get this through.

[chriscerie]

I’m currently ooo, I’ll take a look at the changes within the next week or so.

[chriscerie]

@amitds1997 Are you able to upload the builds under your fork's release? I think that's needed to test selene-github.

[amitds1997]

@amitds1997 Are you able to upload the builds under your fork's release? I think that's needed to test selene-github.

I was able to test it out with this:

  - repo: https://github.com/amitds1997/selene
    rev: d895ab518a9a3fd6e8efe1716fde501dd29fe8ba # The latest commit on the PR branch
    hooks:
      - id: selene-github

Did not face any issues. The reason I think this should work is that it looks for releases under Kampfkarren/selene's releases (see amitds1997/selene/pyproject.toml#L6). Are you seeing any particular error?

[chriscerie]

Are you seeing any particular error?

It works when I have a local copy of selene, but it's breaking when it's not installed:

selene (GitHub)..........................................................Failed
- hook id: selene-github
- exit code: 1

Executable `selene` not found

You're right about it using git-url to get the upstream repo. Perhaps where it's breaking is it's missing extract-files (https://github.com/JohnnyMorganz/StyLua/blob/main/pyproject.toml#L7) and it's falling back to a local copy of selene if installed?

[IamTheFij]

Hello. I just started looking into Selene and noticed that this is kind of waiting on me! Sorry about that. I just pushed a new version that should resolve this issue and allow the GitHub install hook to properly download.

[amitds1997]

@chriscerie @jenstroeger With the latest release to release-gitter (thanks @IamTheFij), we have a working solution for pip-based pre-commit installation.

With this, we should have pre-commit support using local exec, docker and pip (python). Let me know if anything else would be needed from my end.

[IamTheFij]

These should no longer be needed. This was common enough that I added it as default values. That said, there is no harm in leaving them.

https://github.com/IamTheFij/release-gitter/blob/main/release_gitter.py#L51

SYSTEM_SYNONYMS: list[list[str]] = [
    ["Darwin", "darwin", "MacOS", "macos", "macOS"],
    ["Windows", "windows", "win", "win32", "win64"],
    ["Linux", "linux"],
]

[amitds1997]

Thanks for the input! Removed this in the latest commit (391b18d).

[IamTheFij]

Looks good!