search

mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
6.99k stars 333 forks source link

Add `pre-commit.com` support #966

Closed taoufik07 closed 1 year ago

taoufik07 commented 1 year ago

Now that language_version is supported for golang starting from pre-commit v3.0.0 and as this was the main concern, I think we can now fully support to pre-commit.

mvdan commented 1 year ago

Thanks for the pull request. It's good to see that pre-commit is improving its support for Go. That said, I'm still puzzled as to why I need to add a YAML file at the top of the repository to define pre-commit hooks. If anyone wants to run shfmt and they have Go installed, it is as easy as:

go run mvdan.cc/sh/v3/cmd/shfmt@latest --version

It seems to me like tools like pre-commit could support running any Go program as a hook without the upstream having to define a YAML file. Upstreams already declare that they are a Go module, and that should be enough. I do not mean to be dismissive, but I also don't see a point in having a top-level file that does little more than say "look, I can run as a pre-commit hook" :)

trallnag commented 1 year ago

Advantage of pre-commit is that it installs Golang. So it is easier to use shfmt in a non Go project. But the workaround with a forked project works fine as well.

trallnag commented 1 year ago

@taoufik07 the alternative is to mirror the project manually like I did here: https://github.com/trallnag/pre-commit-mirror-gofumpt

Freed-Wu commented 1 year ago

Comes from #976. Now pre-commit-mirror-maker doesn't support go. See https://github.com/pre-commit/pre-commit-mirror-maker/issues/164.

Freed-Wu commented 1 year ago

BTW, shfmt doesn't support zsh. So the following is needed:

  exclude_types:
    - zsh
mvdan commented 1 year ago

Advantage of pre-commit is that it installs Golang.

If pre-commit already knows how to install Go, presumably it could also know how to use go run :) As a user of both pre-commit and shfmt, all you should really have to say is: run the Go program at mvdan.cc/sh/v3/cmd/shfmt at version v3.6.0 with the flags -s -w. That is so little information that it could fit into a few lines of YAML config for those people wishing to use shfmt with pre-commit.

Or, if people really want something that works with the current pre-commit, https://github.com/scop/pre-commit-shfmt already exists.

I understand it would be easy for me to just merge this PR, because multiple people would clearly like that. But I still don't really see why such a file needs to exist, and there is an easy workaround that works today, so I still lean towards leaving things the way they are for now.