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.

Sign up for GitHub

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

Switch to gci, so we can actually auto-group imports #5493

Merged
merged 26 commits into from
Dec 21, 2023
Merged

Switch to gci, so we can actually auto-group imports #5493

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
4ff2686
Switch to gofancyimports, so we can actually auto-group imports
Groxx Dec 18, 2023
7665b06
reformatting all files
Groxx Dec 18, 2023
4fa204e
Merge branch 'master' into imports
Groxx Dec 18, 2023
9c7a534
re-formatting
Groxx Dec 18, 2023
19eee6c
flaky test
Groxx Dec 18, 2023
2e9b822
derp
Groxx Dec 18, 2023
1b4a1cc
host integration is also racing in loggers
Groxx Dec 19, 2023
2b657ea
fixing build, largely a guess though tbh
Groxx Dec 19, 2023
a19b6d0
ugh. I always forget that go build ./... does not build tests.
Groxx Dec 19, 2023
991ad4b
well. gofancyimports does not remove unused imports.
Groxx Dec 19, 2023
968093f
Merge branch 'master' into imports
Groxx Dec 19, 2023
d099971
cleanup
Groxx Dec 19, 2023
f4fa916
Switch to gci. A bit more configurable and in golangci-lint
Groxx Dec 19, 2023
ddfc80e
quiet the thrashing on internal/tools
Groxx Dec 19, 2023
1bdd7f8
Merge branch 'master' into imports
Groxx Dec 19, 2023
d95cef7
unrelated change
Groxx Dec 19, 2023
74712e8
undo host/ test reverts, try from master
Groxx Dec 20, 2023
e9db9b5
Merge branch 'master' into imports
Groxx Dec 20, 2023
d65b4f5
fmt
Groxx Dec 20, 2023
b310308
reverting test-revert
Groxx Dec 20, 2023
ba13b03
fix "no changes" linter hopefully
Groxx Dec 20, 2023
35ba535
uh. prefix redirection?
Groxx Dec 20, 2023
554f600
aah. right. needs to be merged before it takes effect.
Groxx Dec 20, 2023
a14c7e2
Merge branch 'master' into imports
Groxx Dec 20, 2023
62528e6
fmt
Groxx Dec 20, 2023
5ccb6ad
Merge branch 'master' into imports
Groxx Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gen/proto/history/v1/service.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion .gen/proto/indexer/v1/messages.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .gen/proto/matching/v1/service.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion .gen/proto/shared/v1/error.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion .gen/proto/shared/v1/history.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 20 additions & 10 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ default: help
# temporary build products and book-keeping targets that are always good to / safe to clean.
BUILD := .build
# bins that are `make clean` friendly, i.e. they build quickly and do not require new downloads.
# in particular this should include goimports, as it changes based on which version of go compiles it,
# in particular this should include gofmt, as it changes based on which version of go compiles it,
# and few know to do more than `make clean`.
BIN := $(BUILD)/bin
# relatively stable build products, e.g. tools.
Expand All @@ -29,7 +29,7 @@ STABLE_BIN := .bin


# current (when committed) version of Go used in CI, and ideally also our docker images.
# this generally does not matter, but can impact goimports output.
# this generally does not matter, but can impact gofmt output.
# for maximum stability, make sure you use the same version as CI uses.
#
# this can _likely_ remain a major version, as fmt output does not tend to change in minor versions,
Expand Down Expand Up @@ -180,8 +180,15 @@ $(BIN)/mockery: internal/tools/go.mod
$(BIN)/enumer: internal/tools/go.mod
$(call go_build_tool,github.com/dmarkham/enumer)

$(BIN)/goimports: internal/tools/go.mod
$(call go_build_tool,golang.org/x/tools/cmd/goimports)
# it's important that the formatter is NOT whatever version they have installed,
# so build it for this project.
# prefer go_build_tool to support newer go versions, but either is reasonable.
$(BIN)/gofmt: internal/tools/go.mod
$(call go_build_tool,cmd/gofmt)
Copy link
Member Author

@Groxx Groxx Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

gofancyimports doesn't format, and it conflicts with goimports (which does format).

so now we have "normal" gofmt use.

perhaps we should add -s to the gofmt command? all it's doing at the moment is removing a bunch of redundant types like

- MaxRetentionDays: DynamicInt{
+ MaxRetentionDays: {

some of which are ever-so-slightly odd looking to my eyes, though it follows the same pattern:

- ... map[string]time.Time{clusterName: time.Time{}},
+ ... map[string]time.Time{clusterName: {}},

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is the issues with fancyimports. I wonder how we can use the same logic in IDEs.
Maybe we can create a wrapper that does goimports + fancyimports afterwards?

Copy link
Member Author

@Groxx Groxx Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

to be clear, removing goimports and using gofmt is not a problem. this is just the motivation. gofmt is faster than goimports, and gofancyimports takes care of the imports that gofmt doesn't touch.

Copy link
Member Author

@Groxx Groxx Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ah, for IDEs: should definitely be possible, though unlike goimports it needs to be in addition to gofmt, it's not a replacement. combining those might require a small wrapper-binary 🤔 . at the very least though, them doing different things does mean that the order doesn't matter, which is good.

though since it's an Analyzer, it should integrate into gopls just fine.

a combined-binary might be reasonable to add to gofancyimports, for IDE purposes. GoLand has a "goimports-like" formatting option which acts a lot like this too.

Copy link
Member

Choose a reason for hiding this comment

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

After this lands, what should we all use as Format Tool in IDE?

Copy link
Member Author

@Groxx Groxx Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just goimports with -local afaict. I honestly disable this though, IDE formatting does funky and incorrect things kinda frequently.

Or the same thing as the makefile does, if you want true stability and it can run arbitrary binaries

Copy link
Member Author

Choose a reason for hiding this comment

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

from some side chats: we'll probably bundle a formatter-wrapper to simplify vscode and maybe other tools.

that's a bit easier to discover and fix after it's merged though, so I'm just going to skip it for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

similar stuff with gci due to unused-import cleanup, but this does also gofmt so a custom wrapper to run both is probably less needed - IDEs often detect and warn and offer to clean those up separately.


# only organizes imports, does not reformat code
$(BIN)/gofancyimports: internal/tools/go.mod
$(call go_build_tool,github.com/NonLogicalDev/gofancyimports/cmd/gofancyimports)

$(BIN)/gowrap: go.mod
$(call go_build_tool,github.com/hexdigest/gowrap/cmd/gowrap)
Expand Down Expand Up @@ -352,10 +359,13 @@ $(BUILD)/lint: $(LINT_SRC) $(BIN)/revive | $(BUILD)
# if either changes, this will need to change.
MAYBE_TOUCH_COPYRIGHT=

$(BUILD)/fmt: $(ALL_SRC) $(BIN)/goimports | $(BUILD)
$Q echo "goimports..."
$Q # use FRESH_ALL_SRC so it won't miss any generated files produced earlier
$Q $(BIN)/goimports -local "github.com/uber/cadence" -w $(FRESH_ALL_SRC)
# use FRESH_ALL_SRC so it won't miss any generated files produced earlier.
$(BUILD)/fmt: $(ALL_SRC) $(BIN)/gofmt $(BIN)/gofancyimports | $(BUILD)
$Q echo "grouping imports..."
$Q $(BIN)/gofancyimports fix --local github.com/uber/cadence/ --write $(FRESH_ALL_SRC)
Groxx marked this conversation as resolved.
Show resolved Hide resolved
$Q # gofancyimports does not -w code, so also run gofmt
$Q echo "formatting..."
$Q $(BIN)/gofmt -w $(FRESH_ALL_SRC)
$Q touch $@
$Q $(MAYBE_TOUCH_COPYRIGHT)

Expand Down Expand Up @@ -386,8 +396,8 @@ endef
lint: ## (re)run the linter
$(call remake,proto-lint lint)

# intentionally not re-making, goimports is slow and it's clear when it's unnecessary
fmt: $(BUILD)/fmt ## run goimports
# intentionally not re-making, it's a bit slow and it's clear when it's unnecessary
fmt: $(BUILD)/fmt ## run gofmt / organize imports / etc

# not identical to the intermediate target, but does provide the same codegen (or more).
copyright: $(BIN)/copyright | $(BUILD) ## update copyright headers
Expand Down
3 changes: 1 addition & 2 deletions canary/crosscluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@ import (
"fmt"
"time"

shared "go.uber.org/cadence/.gen/go/shared"

"github.com/google/uuid"
shared "go.uber.org/cadence/.gen/go/shared"
"go.uber.org/cadence/activity"
"go.uber.org/cadence/workflow"
"go.uber.org/zap"
Expand Down
10 changes: 4 additions & 6 deletions canary/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@
package canary

import (
"fmt"
"sync"
"time"

"crypto/tls"
"crypto/x509"
"fmt"
"io/ioutil"
"sync"
"time"

apiv1 "github.com/uber/cadence-idl/go/proto/api/v1"
"go.uber.org/cadence/.gen/go/cadence/workflowserviceclient"
"go.uber.org/cadence/compatibility"
"go.uber.org/yarpc"
Expand All @@ -40,8 +40,6 @@ import (
"go.uber.org/zap"
"google.golang.org/grpc/credentials"

apiv1 "github.com/uber/cadence-idl/go/proto/api/v1"

"github.com/uber/cadence/common/log/loggerimpl"
)

Expand Down
2 changes: 1 addition & 1 deletion client/admin/grpcClient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ package admin
import (
"context"

adminv1 "github.com/uber/cadence-idl/go/proto/admin/v1"
"go.uber.org/yarpc"

adminv1 "github.com/uber/cadence-idl/go/proto/admin/v1"
"github.com/uber/cadence/common/types"
"github.com/uber/cadence/common/types/mapper/proto"
)
Expand Down
6 changes: 2 additions & 4 deletions client/clientfactory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,16 @@ package client
import (
"time"

adminv1 "github.com/uber/cadence-idl/go/proto/admin/v1"
apiv1 "github.com/uber/cadence-idl/go/proto/api/v1"
"go.uber.org/yarpc/api/transport"

"github.com/uber/cadence/.gen/go/admin/adminserviceclient"
"github.com/uber/cadence/.gen/go/cadence/workflowserviceclient"
"github.com/uber/cadence/.gen/go/history/historyserviceclient"
"github.com/uber/cadence/.gen/go/matching/matchingserviceclient"

adminv1 "github.com/uber/cadence-idl/go/proto/admin/v1"
apiv1 "github.com/uber/cadence-idl/go/proto/api/v1"
historyv1 "github.com/uber/cadence/.gen/proto/history/v1"
matchingv1 "github.com/uber/cadence/.gen/proto/matching/v1"

"github.com/uber/cadence/client/admin"
"github.com/uber/cadence/client/frontend"
"github.com/uber/cadence/client/history"
Expand Down
2 changes: 1 addition & 1 deletion client/frontend/grpcClient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ package frontend
import (
"context"

apiv1 "github.com/uber/cadence-idl/go/proto/api/v1"
"go.uber.org/yarpc"

apiv1 "github.com/uber/cadence-idl/go/proto/api/v1"
"github.com/uber/cadence/common/types"
"github.com/uber/cadence/common/types/mapper/proto"
)
Expand Down
3 changes: 1 addition & 2 deletions cmd/server/cadence/cadence.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,12 @@ import (
"strings"
"syscall"

"github.com/uber/cadence/common/persistence/nosql/nosqlplugin/cassandra/gocql"

"github.com/urfave/cli"

"github.com/uber/cadence/common"
"github.com/uber/cadence/common/client"
"github.com/uber/cadence/common/config"
"github.com/uber/cadence/common/persistence/nosql/nosqlplugin/cassandra/gocql"
"github.com/uber/cadence/common/service"
"github.com/uber/cadence/tools/cassandra"
"github.com/uber/cadence/tools/sql"
Expand Down
13 changes: 5 additions & 8 deletions cmd/server/cadence/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,11 @@ import (
"log"
"time"

"github.com/startreedata/pinot-client-go/pinot"
apiv1 "github.com/uber/cadence-idl/go/proto/api/v1"
"go.uber.org/cadence/.gen/go/cadence/workflowserviceclient"
"go.uber.org/cadence/compatibility"

apiv1 "github.com/uber/cadence-idl/go/proto/api/v1"
"github.com/uber/cadence/common/persistence"
"github.com/uber/cadence/service/worker"

"github.com/uber/cadence/common"
"github.com/uber/cadence/common/archiver"
"github.com/uber/cadence/common/archiver/provider"
Expand All @@ -46,16 +44,15 @@ import (
"github.com/uber/cadence/common/messaging/kafka"
"github.com/uber/cadence/common/metrics"
"github.com/uber/cadence/common/peerprovider/ringpopprovider"
"github.com/uber/cadence/common/persistence"
pnt "github.com/uber/cadence/common/pinot"
"github.com/uber/cadence/common/resource"
"github.com/uber/cadence/common/rpc"
"github.com/uber/cadence/common/service"
"github.com/uber/cadence/service/frontend"
"github.com/uber/cadence/service/history"
"github.com/uber/cadence/service/matching"

"github.com/startreedata/pinot-client-go/pinot"

pnt "github.com/uber/cadence/common/pinot"
"github.com/uber/cadence/service/worker"
)

type (
Expand Down
4 changes: 1 addition & 3 deletions common/archiver/URI.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@

package archiver

import (
"net/url"
)
import "net/url"

type (
// URI identifies the archival resource to which records are written to and read from.
Expand Down
4 changes: 1 addition & 3 deletions common/archiver/archivalMetadata_mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@

package archiver

import (
"github.com/stretchr/testify/mock"
)
import "github.com/stretchr/testify/mock"

// MockArchivalMetadata is an autogenerated mock type for the ArchivalMetadata type
type MockArchivalMetadata struct {
Expand Down
4 changes: 1 addition & 3 deletions common/archiver/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@

package archiver

import (
"errors"
)
import "errors"

const (
// ArchiveNonRetriableErrorMsg is the log message when the Archive() method encounters a non-retriable error
Expand Down
5 changes: 2 additions & 3 deletions common/archiver/gcloud/connector/mocks/BucketHandleWrapper.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions common/archiver/gcloud/connector/mocks/Client.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions common/archiver/gcloud/connector/mocks/ObjectHandleWrapper.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 1 addition & 2 deletions common/archiver/provider/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@ import (
"errors"
"sync"

"github.com/uber/cadence/common/archiver/gcloud"

"github.com/uber/cadence/common/archiver"
"github.com/uber/cadence/common/archiver/filestore"
"github.com/uber/cadence/common/archiver/gcloud"
"github.com/uber/cadence/common/archiver/s3store"
"github.com/uber/cadence/common/config"
)
Expand Down
3 changes: 1 addition & 2 deletions common/archiver/s3store/mocks/S3API.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion common/archiver/s3store/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import (
"go.uber.org/multierr"

"github.com/uber/cadence/common"

"github.com/uber/cadence/common/archiver"
"github.com/uber/cadence/common/types"
)
Expand Down
3 changes: 1 addition & 2 deletions common/archiver/s3store/visibilityArchiver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ package s3store
import (
"context"

"github.com/uber/cadence/common/metrics"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/s3"
Expand All @@ -33,6 +31,7 @@ import (
"github.com/uber/cadence/common/archiver"
"github.com/uber/cadence/common/config"
"github.com/uber/cadence/common/log/tag"
"github.com/uber/cadence/common/metrics"
"github.com/uber/cadence/common/types"
)

Expand Down
3 changes: 1 addition & 2 deletions common/backoff/cron_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,8 @@ import (
"testing"
"time"

"github.com/stretchr/testify/require"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestCron(t *testing.T) {
Expand Down
4 changes: 1 addition & 3 deletions common/blobstore/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@

package blobstore

import (
"context"
)
import "context"

type (
// Client defines the interface to a blobstore client.
Expand Down
1 change: 0 additions & 1 deletion common/cache/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"time"

"github.com/uber/cadence/common"

"github.com/uber/cadence/common/metrics"
)

Expand Down
Loading