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

chore: add goimports #20651

Merged
merged 1 commit into from
Jan 29, 2021
Merged

chore: add goimports #20651

merged 1 commit into from
Jan 29, 2021

Conversation

lesam
Copy link
Contributor

@lesam lesam commented Jan 29, 2021

Closes #

@lesam
Copy link
Contributor Author

lesam commented Jan 29, 2021

@lesam lesam requested a review from danxmoran January 29, 2021 17:13
Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raging against the machine

@lesam lesam force-pushed the add-goimports-2 branch 2 times, most recently from b4520c7 to 9b78159 Compare January 29, 2021 18:42
@lesam
Copy link
Contributor Author

lesam commented Jan 29, 2021

It turns out goimports just splits std libraries to the top, and all other groups are just sorted.

Manually fixed the grouping for some files.

@@ -136,7 +136,7 @@ func (c *TagValueSeriesIDCache) Put(name, key, value []byte, ss *tsdb.SeriesIDSe

// No map for the measurement - first tag key for the measurement.
c.cache[string(name)] = map[string]map[string]*list.Element{
string(key): map[string]*list.Element{string(value): listElement},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the gofmt -s - I guess nobody ran a make fmt recently

@lesam lesam requested a review from a team as a code owner January 29, 2021 19:07
HAS_FMT_ERR=0
# For every Go file in the project, excluding vendor...
for file in $(go list -f '{{$dir := .Dir}}{{range .GoFiles}}{{printf "%s/%s\n" $dir .}}{{end}}' ./...); do

for file in $(go list -f '{{$dir := .Dir}}{{range .GoFiles}}{{printf "%s/%s\n" $dir .}}{{end}}{{range .TestGoFiles}}{{printf "%s/%s\n" $dir .}}{{end}}{{range .IgnoredGoFiles}}{{printf "%s/%s\n" $dir .}}{{end}}{{range .CgoFiles}}{{printf "%s/%s\n" $dir .}}{{end}}' ./... ); do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even more fun - we were not doing format checking on tests, cgo files, or ignored files like tools.go

Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My GoLand linter will be very happy after this change

# ... if file does not contain standard generated code comment (https://golang.org/s/generatedcode)...
if ! grep -Exq '^// Code generated .* DO NOT EDIT\.$' $file; then
FMT_OUT="$(gofmt -l -d -e $file)" # gofmt exits 0 regardless of whether it's formatted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep the gofmt call if goimports only checks imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goimports also does formatting . The only thing it is missing is the -s argument to gofmt - do we want to block PR's on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also golang/go#21476

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note we weren't blocking on -s before.

@lesam lesam merged commit d5aa736 into influxdata:master Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants