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

Check if incoming code is gofmt-ed #817

Closed
jkremser opened this issue Dec 23, 2021 · 5 comments
Closed

Check if incoming code is gofmt-ed #817

jkremser opened this issue Dec 23, 2021 · 5 comments
Milestone

Comments

@jkremser
Copy link
Member

jkremser commented Dec 23, 2021

Currently neither the golangci-lint run nor the gokart scan --globalsTainted --verbose do catch some things that gofmt -w -s . does to code.
We can do gofmt -l . that lists all the files that are not aligned w/ the gofmt (~something like a dry run) and check if the output is empty.

Currently it's complaining about terrratest:

 gofmt -l .
terratest/test/init.go
terratest/test/k8gb_abstract_full_roundrobin_test.go
terratest/test/k8gb_basic_app_test.go
terratest/test/k8gb_failover_playground_test.go
terratest/test/k8gb_full_failover_test.go
terratest/test/k8gb_full_roundrobin_test.go
terratest/test/k8gb_ingress_annotation_failover_test.go
terratest/test/k8gb_ingress_annotation_rr_test.go
terratest/test/k8gb_lifecycle_test.go
terratest/test/k8gb_split_failover_test.go

I've checked couple of those and it's my work, perhaps I will setup also some pre-commit hook w/ gofmt. We could have this also in the repo itself, but it might be controversial.

note:
gofmt has an option -s that can simplify the code and these wannabe modifications are not listed w/ -l, but these can be checked by gofmt -s -d . (again if the output is empty, we are good)

@jkremser
Copy link
Member Author

jkremser commented Dec 23, 2021

ah, I've completely missed this https://github.com/k8gb-io/k8gb/blob/master/.golangci.yaml#L43 so it looks like it's on purpose. I wonder why though.

also probably dupe of #775

@kuritka
Copy link
Collaborator

kuritka commented Dec 23, 2021

@jkremser Goimports does everything that gofmt does. Additionally it adds missing imports and removes unreferenced imports , according to this article.

I found one warning with enabled gofmt -s :

# .golangci.yaml
# ...
  - gofmt
# ...
  gofmt:
    # gofmt -s
    simplify: true

The question is, if we must keep both - goimports + gofmt or goimports can be configured to replace gofmt -s.

@jkremser
Copy link
Member Author

I see, so somehow the golangci-lint run is not being run against the terratest module.

I'd probably whitelisted the gofmt, they don't fight against each other and can be run both (also the output is not duplicated), wdyt?

right now, it reports

main.go:41: File is not `goimports`-ed (goimports)
	version       =  "development"

which is kind of misleading

should be (if gofmt is enabled)

main.go:41: File is not `gofmt`-ed with `-s` (gofmt)
	version       =  "development"

@jkremser
Copy link
Member Author

jkremser commented Dec 23, 2021

wrt terratest
cd terratest/ && golangci-lint run ... works (but I assume it would not inherit the options from /.golangci.yaml this way)

@somaritane somaritane added this to the 1.1 milestone Oct 9, 2022
@ytsarev
Copy link
Member

ytsarev commented Jul 20, 2024

closing as solved, we can reopen if linting brings troubles again

@ytsarev ytsarev closed this as completed Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

4 participants