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

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

Merged
merged 26 commits into from
Dec 21, 2023

Conversation

Groxx
Copy link
Member

@Groxx Groxx commented Dec 18, 2023

If you are conflicting with this PR

I would recommend simply recreating its changes, it is likely to be MUCH simpler.
Before you merge or rebase, do this:

# get these makefile + tool changes
git checkout master -- internal/tools Makefile

# force a re-format
rm .build/fmt
make fmt

# get rid of the from-main changes
git checkout HEAD -- internal/tools Makefile

# now rebase or merge

We've finally found a tool that enforces groups, not just sorts them!
Awesome.
Unfortunately it doesn't (currently?) remove unused imports, so we also
still want/need goimports.
Oh well.

We've finally found a tool that enforces groups, not just sorts them!
Awesome.

There are perhaps some tweaks we'd prefer to make (e.g. comment grouping
appears to be custom, and does not perfectly follow the astutil.NewCommentMap
logic), but overall it looks excellent.
@Groxx Groxx changed the title Adopt gofancyimports Switch to gofancyimports, so we can actually auto-group imports Dec 18, 2023
Makefile Outdated
Comment on lines 186 to 187
$(BIN)/gofmt: internal/tools/go.mod
$(call go_build_tool,cmd/gofmt)
Copy link
Member Author

@Groxx Groxx Dec 18, 2023

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

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

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

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.

internal/tools/tools.go Outdated Show resolved Hide resolved
Copy link
Member

@taylanisikdemir taylanisikdemir left a comment

Choose a reason for hiding this comment

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

thanks for trying this import organizer and making bulk changes!

host/integrationbase.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@Groxx Groxx changed the title Switch to gofancyimports, so we can actually auto-group imports Switch to gci, so we can actually auto-group imports Dec 19, 2023
@Groxx Groxx merged commit bae97d5 into cadence-workflow:master Dec 21, 2023
@Groxx Groxx deleted the imports branch December 21, 2023 03:06
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.

3 participants