Add gofmt -s to CI

[jojo43]

Hi, this is my first contribution.
Please feedback me if there's anything.

This PR resolves a part of #108 .

[hashicorp-cla]

All committers have signed the CLA.

[radeksimko]

Thank you for the PR.

I reckon it's sensible to run these checks only on Linux, assuming that gofmt behaves consistently the same on Windows/macOS/Linux.

It seems that this won't be solved natively in gofmt any time soon: golang/go#24230
and the only recommendation seems Linux/Unix-specific (test -z "$(gofmt -s -l "$file")")

I'm not sure about introducing platform-specific logic into the makefile as we just removed it recently in #98 but arguably Make is a unix tool anyway, so maybe that's expected? 🤔

[radeksimko]

I think this won't work on Windows, which I suspect is the reason the first commit has failed CI checks.

[radeksimko]

This won't work on Windows I think

[jojo43]

Thank you for the review. 😃

How about simply not using Makefile like this?

[radeksimko]

How about simply not using Makefile like this?

I'm not opposed to it, but I'm also thinking about this from the perspective of a new contributor. If we enforce some checks in the CI, then contributors should have an easy way of (a) checking that their contribution complies, and (b) fix code that isn't compliant - and do this before they make that contribution.

Right now it appears that Windows users would be left with the only option of either using IDE which does the appropriate formatting for them, or create their own PowerShell/other script. While I'm not Windows user myself, I think that's suboptimal.

[jojo43]

If we enforce some checks in the CI, then contributors should have an easy way of (a) checking that their contribution complies, and (b) fix code that isn't compliant - and do this before they make that contribution.

Make sense.

My another idea is using golangci-lint.
It needs installation step, but could be run on any OS by the same command.

[radeksimko]

Yeah, I think that's a decent solution, but I'd like to first understand all the complexity introduced by golangci-lint and all the other things it does.

My understanding is that it has some opinions on how code should be written and I wouldn't want to treat these opinions as a barrier for contributions to the language server. If CI does flag up something I'd like it to be really justified and I'd like to be sure that these are genuine problems or serious risks.

With that in mind the -s flag of gofmt is an opinion which does not seem to be shared by go fmt, so I'm already on the edge of whether to enforce that in CI checks, but I can be convinced about that more easily than about any linter, assuming we can provide a way for contributors to auto-format the code easily.

[radeksimko]

I haven't tried this myself, but https://github.com/mh-cbon/go-fmt-fail looks like something I would otherwise build in this situation, so that may be worth checking too.

[jojo43]

My understanding is that it has some opinions on how code should be written and I wouldn't want to treat these opinions as a barrier for contributions to the language server. If CI does flag up something I'd like it to be really justified and I'd like to be sure that these are genuine problems or serious risks.

Thank you for your polite comment.
I want to respect your thoughts.

I haven't tried this myself, but https://github.com/mh-cbon/go-fmt-fail looks like something I would otherwise build in this situation, so that may be worth checking too.

I've tried go-fmt-fail in my forked repository ( jojo43#3 ), and it worked as same even on Windows.
So, if you don't mind if -s flag can't be used, I think it is reasonable solution.

[radeksimko]

Sounds great - feel free to modify your PR to use that utility then 👍

Just a few suggestions wrt the linked PR in your repository

(1) I believe it can be significantly simplified by invoking the tool via go run github.com/mh-cbon/go-fmt-fail instead of running the binary directly.

(2) Most Go commands (including go test and go run) support -mod=vendor flag which forces them to only look into the vendor folder - that means we can avoid reinstalling it in every CI check.

That flag is something we should (and currently don't) generally use for all Go commands in CI/makefile to ensure reproducibility and consistency.

That can be fixed by setting GOFLAGS variable to -mod=vendor in the Makefile, e.g. by adding the following line at the top

export GOFLAGS = -mod=vendor

(which seems to work on Windows too FWIW)

and by setting the same variable in the CI via env.

(3)The failure you saw earlier in your Windows CI run is most likely related to the default git settings on Windows where it converts \n to \r\n (i.e. adds carriage return characters). This can (and IMO should) be turned off for all builds via git config --global core.autocrlf false prior to the checkout step.

[jojo43]

Thank you for your advice. 😃
I've learned a lot from you.

I added some fixes.
Please take another look.

[radeksimko]

Thank you for making these last changes and for your contribution overall. 👍

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.