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

consider the main module path when grouping imports #187

Closed
glightfoot opened this issue Jan 27, 2022 · 6 comments · Fixed by #199
Closed

consider the main module path when grouping imports #187

glightfoot opened this issue Jan 27, 2022 · 6 comments · Fixed by #199

Comments

@glightfoot
Copy link

I know this has been discussed before, but it would be EXTREMELY useful to have an option to disable gofumpt's import sorting. We have an old go codebase that does not have a dot in the module name, and this makes gofumpt unusable for us. While the spec makes sense, changing it is not possible at this time. Additionally, we have extra import grouping rules beyond the local prefix that we would like to not lose.

Gofumpt is the best formatter I've found, and the integration with golangci-lint and autofix would be massively useful if we could just disable import sorting and use our own. It sounds like many people are in the same boat, and adding the option would make gofumpt more useful for everyone.

I'm willing to make the PR to add the option if that helps.

Thanks for making such a wonderful tool!

@mvdan
Copy link
Owner

mvdan commented Jan 27, 2022

While the spec makes sense, changing it is not possible at this time. Additionally, we have extra import grouping rules beyond the local prefix that we would like to not lose.

Can you please explain the background there?

As for the option: sorry, but no; I'm trying really hard to not add more knobs. Just one or two more options and the tool gets significantly harder to maintain and less useful to use, as each project would end up with its own variant.

I really do appreciate the kind words and the struggle, but also, this is a heavily opinionated tool by design. Its format is even stricter than gofmt's. I am fully aware that it may be incompatible with what a minority of codebases want to do, and I think that's fine. I seem to recall that there was also significant pushback against gofmt in the first few years. This is not to say that gofumpt should become the next gofmt, but you also aren't forced to obey its rules :)

And, at least in the case of imports, grouping and sorting imports is useful, so I definitely don't want to outright remove that feature.

@glightfoot
Copy link
Author

Sorry, I meant changing our module name is not feasible. From some reading I did in golang issues, it seems like the "all non-stdlib modules should have a domain" wasn't really a rule or anything official, and quite a few older codebases didn't have a domain.

I definitely get wanting to avoid adding knobs and it becoming a maintenance issue, but it does feel like the imports one if different enough to warrant an option to disable that sorting rule, given how many standards and preferences there are around import sorting (see #38). I think this would make adopting gofumpt easier for many people in the same boat.

While the rest of the gofumpt rules are style changes (and all good ones IMO), the imports sorting introduces that unofficial spec that all third party modules should have a domain, which feels different from the rest. Nothing in gofmt/goimports forces modules to be named a certain way, whereas this rule does

@mvdan
Copy link
Owner

mvdan commented Feb 2, 2022

unofficial spec

Just to be clear, it's not an unofficial spec. It is an undocumented assumption which will start being properly documented very soon; see https://go-review.googlesource.com/c/website/+/359594/. I agree it's unfortunate that it was undocumented for a long time, and I appreciate that some projects may struggle with this, but I am not inventing this rule out of thin air :)

That said, perhaps there's a way we could do the right thing without needing a knob. Since gofumpt is already module-aware, it could look up the main module's path; if it is like module foo/bar instead of module foo.com/bar, then it could remember that foo/... is not part of the standard library. Would that be enough for your situation?

@glightfoot
Copy link
Author

Oh that's a great idea! Yeah I think that would solve all the issues, thanks. Out of curiosity, how does gofumpt find the module?

@mvdan
Copy link
Owner

mvdan commented Feb 2, 2022

out, err := exec.Command("go", "list", "-m", "-f", "{{.GoVersion}}").Output()

At the moment we only look up the go X.Y information, but also looking up the module path would be rather easy.

@glightfoot
Copy link
Author

Yup! I just confirmed that excluding imports beginning with the path from go list -m -f {{.Path}} from import sorting would fix this entirely. Thanks again

@mvdan mvdan changed the title Option to disable import sorting consider the main module path when grouping imports Feb 5, 2022
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.

2 participants