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

Optional flag to disable all import sorting? #120

Closed
sk0g opened this issue Apr 16, 2021 · 10 comments · Fixed by #199
Closed

Optional flag to disable all import sorting? #120

sk0g opened this issue Apr 16, 2021 · 10 comments · Fixed by #199

Comments

@sk0g
Copy link

sk0g commented Apr 16, 2021

I'm sure the default settings work for many, but when looking at this tool, it doesn't let you set a local prefix, like goimports does. As such, I am looking for a way to use goimports in conjunction with gofumpt.

I set them both up as file watchers in GoLand, but the issue there is that goimports is much faster, and always finishes first. Which makes the file's imports nice for a second. Then gofumpt finishes, and undoes all the imports sorting done by goimports, with it's mangled version.

So for example, goimports -local abc -w $FilePath formats like this:

import (
    "net/http"

    "abc"
    "abc/xyz"

    "github.com/external/package"
)

gofumpt formats it like this:

import (
    "abc"
    "abc/xyz"
    "net/http"

    "github.com/external/package"
)

So is there a way to skip the imports sorting entirely, and leave that to goimports? If not, and if it's doable, I would appreciate pointers as to how to get that done!

Ideally this just passes on the local arg to goimports, which might solve the issue of overwriting as well. Even if this didn't touch the imports block at all, I'm not entirely sure if the conflicting finish times would be an issue or not still.

PS I love the tool, and have been manually enforcing a few of the rules already, and agree with most of the other rules. Just a shame it's incompatible with how the project currently formats imports, or we would be using it already :)

@karelbilek
Copy link
Contributor

gofumpt also have gofumports command, which is goimports with gofumpt+goimports. Maybe that has -local? (just a wild guess)

@karelbilek
Copy link
Contributor

Ah, gofumports no longer exists.

38fc491

@sk0g
Copy link
Author

sk0g commented Apr 16, 2021

@karelbilek That would certainly have been worth a look, but it's been removed.

Worst case scenario I could fork it and remove the import sorting functionality entirely, I guess.

@karelbilek
Copy link
Contributor

Anyone who still uses gofumports should consider switching. The
alternatives are running goimports and gofumpt separately, using the
mvdan.cc/gofumpt/format Go API, or sticking to v0.1.0 for a little
while.

I see. :(

@karelbilek
Copy link
Contributor

well one way to your original problem is... to just create a manual script that does both, in the order gofumpt-goimports, and put it to path...

@sk0g
Copy link
Author

sk0g commented Apr 16, 2021

That's what I was trying too actually. In GoLand, chaining them together using either &&, or ; didn't work. Not entirely sure what shell they use for the file watchers, but that may be worth a look into as well.

@mvdan
Copy link
Owner

mvdan commented Apr 16, 2021

What you're running into here is unrelated to -local; it's the rule that joins standard library imports. See #117 (comment). You shouldn't be using module paths like abc, because they are reserved.

I plan on enhancing the feature with #38, but that wouldn't change the logic to join all standard library imports, nor would it mean adding a -local. This tool replaces gofmt after all, not goimports.

Hope that clarifies things. I know it's confusing and perhaps frustrating, but I don't think supporting reserved module paths makes sense in the long run :)

@sk0g
Copy link
Author

sk0g commented Apr 18, 2021

@mvdan the module prefixes aren't actually abc - I was just providing that as an example.

I had a look at this project's code, and noticed something different with the go.mod file - your module name is {domain_name}/{repo_name}, while the project I'm running into this issue with, the module name is just {repo_name}.

I had a quick play around with renaming the module name to start with a domain name, and while it produced a gigantic diff, it seems to play nicely with gofumpt now, so it can be the lone file watcher and makes the code all nice and tidy now.

So this issue is resolved for me now, thanks for the help!

@mvdan
Copy link
Owner

mvdan commented Apr 20, 2021

Right, what I meant is that not using a domain name - not having a dot - means that you're using a path reserved by the Go toolchain, for paths like the standard library. So gofumpt assumes it's std.

Glad that worked for you :) Closing.

@mvdan mvdan closed this as completed Apr 20, 2021
@mvdan
Copy link
Owner

mvdan commented Feb 5, 2022

FYI, see #187 (comment); I'm currently thinking we could do better for everyone by looking at the main module's path.

mvdan added a commit that referenced this issue Feb 21, 2022
Add the feature, test it, and document it in the README.
It's enough to mention it in the FAQ, as I expect that most users will
never run into this edge case. And those few that may run into it will
hopefully never need to know how it works or why.

Note that this adds a new option to pass the info from go.mod,
much like LangVersion already did.

Fixes #22.
Fixes #117.
Fixes #120.
Fixes #187.
mvdan added a commit that referenced this issue Feb 21, 2022
Add the feature, test it, and document it in the README.
It's enough to mention it in the FAQ, as I expect that most users will
never run into this edge case. And those few that may run into it will
hopefully never need to know how it works or why.

Note that this adds a new option to pass the info from go.mod,
much like LangVersion already did.

Fixes #22.
Fixes #117.
Fixes #120.
Fixes #187.
mvdan added a commit that referenced this issue Feb 22, 2022
Add the feature, test it, and document it in the README.
It's enough to mention it in the FAQ, as I expect that most users will
never run into this edge case. And those few that may run into it will
hopefully never need to know how it works or why.

Note that this adds a new option to pass the info from go.mod,
much like LangVersion already did.

Fixes #22.
Fixes #117.
Fixes #120.
Fixes #187.
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 a pull request may close this issue.

3 participants