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

[Pre-commit] Feature Request (Remove Crate-Dependency) #285

Closed
Skylion007 opened this issue Jun 15, 2021 · 10 comments · Fixed by #289
Closed

[Pre-commit] Feature Request (Remove Crate-Dependency) #285

Skylion007 opened this issue Jun 15, 2021 · 10 comments · Fixed by #289
Labels
enhancement Improve the expected

Comments

@Skylion007
Copy link

I really like this project and would like to include it as a pre-commit hook, but asking all my git repo to install Rust for the pre-commit hook is a bit much. As such, would it be possible to wrap the binary in a setup.py like is done by CircleCI-Py and such https://github.com/AleksaC/hadolint-py/blob/main/setup.py . That way it would not need any additional pre-reqs besides the ones for pre-commit.

@epage
Copy link
Collaborator

epage commented Jun 15, 2021

I've personally not used pre-commit, so I've not looked into this too much. For anyone else, https://pre-commit.com/index.html#rust has the details. It basically is having typos built from source.

The given hadolint example is hacking in a binary download via setup.py

Other options:

It'd be nice if there was a gh-install hook type.

@epage
Copy link
Collaborator

epage commented Jun 17, 2021

@scop since you created the hook, any thoughts on this?

@scop
Copy link
Contributor

scop commented Jun 17, 2021

https://github.com/shellcheck-py/shellcheck-py is a similar setup.py based approach, better in the sense that it contains some verification of downloads. If this was my project, I'd personally probably stick with the source based build in this repo to serve audiences who rather don't use binaries from "random" sources, and a binary based one could be hosted in another repo (like shellcheck vs shellcheck-py, ditto hadolint/-py) by whoever is interested. Granted, the binaries here or in the mentioned projects aren't really that "random" but shipped in the respective author repos.

@epage
Copy link
Collaborator

epage commented Jun 17, 2021

I hadn't considered the idea of having a dedicated repo for alternative approaches!

The main annoyance is keeping the version up-to-date (since cargo-release does everything within a single-repo for me automatically)

@scop
Copy link
Contributor

scop commented Jun 18, 2021

One can also have multiple hooks defined in the same .pre-commit-hooks.yaml file -- if so, others could be hosted here in the same file. Haven't tried out one with differing dependency sets/languages between hooks myself, but I hope pre-commit would only install let's say the typos binary based hook if one chooses to use that one and not others along with their dependencies in the same file.

@scop
Copy link
Contributor

scop commented Jun 18, 2021

One thing to notice with a hypothetical setup like this, assuming binaries will be made available only for tagged releases: then a binary based hook would also work only with user configs with their rev pointing to a tag and not a commit hash, unless the setup.py or something has capability to grab the binary corresponding to a hash somewhere else where it's available besides GitHub releases. The source based one does not have this "issue". A rare use case, but good to understand.

@Skylion007
Copy link
Author

No reason you can't have both hooks. One will be a Python one and one will be a crate hook.

@epage
Copy link
Collaborator

epage commented Jun 18, 2021

Hadn't realized users can opt-in to each hook within the file separately. That sounds like the way to go

@epage
Copy link
Collaborator

epage commented Jun 18, 2021

Any preference on naming convention? Or would someone more invested be interested in writing it, since I'd just be writing it blind...

@scop
Copy link
Contributor

scop commented Jun 21, 2021

I won't have time to look into this until about next weekend/next week, but if nobody beats me to it, I think I can have a quick look. I think I'd

  • choose one to be the "primary" and make its id typos, and the other's typos-bin/typos-src (maybe bin as primary, guessing that's what most users would expect)
  • name: typos for both as that's what gets displayed and the variant isn't that important there (a bit torn on this though)
  • note the variant in description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve the expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants