doc: improve documentation for goimport local-prefixes

[sebastien-rosset]

Change in this PR

Improve the documentation of the goimports linter.

Problem Statement

The existing local-prefixes attribute of the goimports linter does not explain what happens if the attribute is set.

The existing summary of the goimports linter is:

In addition to fixing imports, goimports also formats your code in the same style as gofmt.

linters-settings:
  goimports:
    # Put imports beginning with prefix after 3rd-party packages.
    # It's a comma-separated list of prefixes.
    # Default: ""
    local-prefixes: github.com/org/project

[bombsimon]

I think it's great to be clear and have documentation but this is very verbose and no other linter explains what error message is expected when the linter fail. I also feel it's even less important for linters with fixers like goimports has so by just running golangci-lint run --fix the issues will be resolved.

Do you think it would be enough to do this change?

- # Put imports beginning with prefix after 3rd-party packages.
+ # Put imports beginning with prefix as a separate group after 3rd-party packages.

The information that it's after 3rd party packages is already there and with the change above it's clear it will be in a separate group.

[sebastien-rosset]

In the existing golangci-lint doc, it's not easy to relate why goimports is a linter. When I read the documentation at https://golangci-lint.run/usage/linters/, there is an entry for goimports, which links to https://pkg.go.dev/golang.org/x/tools/cmd/goimports, the go formatting tool. This quite unusual. For most (all?) other linter entries, there is a link to an actual linter, where the README file explains what the linter is. But in the case of goimports, it's just a link to the goimports command.

The goimports command is generally understood to be a formatting tool for go imports, not a linter. That creates the initial confusion: why is goimports a linter? It's meant to format code, not to create linter issues.

Furthermore, In the linked https://pkg.go.dev/golang.org/x/tools/cmd/goimports#section-documentation, there is no explanation about local-prefixes, especially in the context of linters. The goimports command does accept a documented -local argument, but its intent is to reformat code, not to raise linter issues.

In the golangci-lint context, the goimports linters does not "put imports beginning with prefix as a separate group". Instead, it creates a linter issue if the existing source code is not grouped as expected. This linter is not meant to be a code reformatting tool.

Finally, afaik this is a comma-separated list of prefixes. There could be more than one prefix.

So overall, there is quite a bit of implied knowledge. This linter would be easier to use with more explicit documentation.

[sebastien-rosset]

what error message is expected when the linter fail.

How about this?

# A comma-separated list of prefixes, which, if set, checks import paths
# with the given prefixes are grouped after 3rd-party packages.
# Unlike the `goimports` command, the `goimports` linter does not reformat code.
# Instead, it checks the imports are grouped as expected.

[bombsimon]

This quite unusual. For most (all?) other linter entries, there is a link to an actual linter

It's the same for gofmt which also links to the documentation view since there is no other link to provde for linters/formatters from stdlib. I think the difference here is built-in go tools and 3rd party linters.

The goimports command is generally understood to be a formatting tool for go imports, not a linter.

The linter is to check if the formatter has been run. The same goes for the gofmt linter. There are other formatters as well such as whitespace and gofumpt that work in the same way. I would also like to add that golangci-lint is also a formatter, that's what the --fix flag is for. It reformats your code according to your selected linters (that checks if your code is formatted according to said formatters). Personally I run goimports on save in my editor but it's totally valid to run golangci-lint --fix on save as well.

The goimports command does accept a documented -local argument, but its intent is to reformat code, not to raise linter issues.

Yeah maybe it's bad that the config parameter and the flag isn't the same. I think the config was used to actually clarify it's just a prefix but I don't know why this was chosen.

I would say reformatting is one of the things it does, but not only. By default it does not, it just reports diffs. You have to specify the -w flag to reformat the code. The same goes for gofmt and other style linters. We even have gci which is basically doing the same job as goimports; it fails if your imports are not in the configured order and it sorts them if you use the --fix flag. So this is a pretty standard thing for golangci-lint.

I might be biased here but I think goimports is no different from a lot of other linters and formatters and a pretty common one since it's from stdlib. Since there are more formatters that formats your code with --fix (or the standalone linter) I don't think goimports documentation should be treated differently, this is already documented under issues configuration and Command-Line Options.

To me, I think your second alternative without the note about formatting vs. checking if formatted sounds best, that is:

# A comma-separated list of prefixes, which, if set, checks import paths
# with the given prefixes are grouped after 3rd-party packages.

[sebastien-rosset]

I also changed the description in pkg/golinters/goimports.go which was a copy-paste from the goimports command. It was wrong. The goimports linter is not the same as the goimports command, it does not reformat code.

[sebastien-rosset]

As a side note, a chore would be to have consistent grammar for the linter doc (https://golangci-lint.run/usage/linters/):

1. Sentence starts with uppercase/lowercase.
2. Start with the name of the linter versus a verb versus noun ("tool", "go linter"...)
3. Verb base form versus 3rd-person singular.

[bombsimon]

Did you see there's a column at https://golangci-lint.run/usage/linters/ that tells if the linter has autofix or not (that it will format your code with --fix which is the same)? I'm not sure it's needed twice? Also worth mentioning the flag is not the only way apply fixes.

[bombsimon]

As a side note, a chore would be to have consistent grammar for the linter doc (https://golangci-lint.run/usage/linters/):

1. Sentence starts with uppercase/lowercase.

2. Start with the name of the linter versus a verb versus noun ("tool", "go linter"...)

3. Verb base form versus 3rd-person singular.

Yeah the documentation is a result of several years and multiple authors, would be nice with an overhaul. Appreciate the improvement but this is one of the reasons I think keeping the docs lean and without duplications is good. Keeping the docs as clear as possible but with fewer details helps reduce maintenance in my opinion. My reviews are just pointers to where the information already exist but if you don't agree that's enough I'm all for improving that.

[sebastien-rosset]

As a side note, a chore would be to have consistent grammar for the linter doc (https://golangci-lint.run/usage/linters/):

1. Sentence starts with uppercase/lowercase.
2. Start with the name of the linter versus a verb versus noun ("tool", "go linter"...)
3. Verb base form versus 3rd-person singular.

Yeah the documentation is a result of several years and multiple authors, would be nice with an overhaul. Appreciate the improvement but this is one of the reasons I think keeping the docs lean and without duplications is good. Keeping the docs as clear as possible but with fewer details helps reduce maintenance in my opinion. My reviews are just pointers to where the information already exist but if you don't agree that's enough I'm all for improving that.

I come from the point of view of consuming golangci-lint and trying to understand the doc. Specifically for the goimports linter, I thought the doc was confusing. I don't think it's the right thing to expect users to have to read the code, know how to put various bits of the doc together and go through trial and error. I didn't realize 4 extra lines of doc would cause maintenance problems and that it was important to favor maintenance over user experience.

[bombsimon]

I don't think it's the right thing to expect users to have to read the code, know how to put various bits of the doc together and go through trial and error.

Yeah maybe not. This has been discussed multiple times and it's tricky with metalinters like golangci-lint. I think you can expect some kind of research and documentation reading from users opting in to use golangci-lint but of course it's better if the onboarding is as streamlined as possible.

I didn't realize 4 extra lines of doc would cause maintenance problems and that it was important to favor maintenance over user experience.

Every line of documentation or code is more maintenance than nothing. In this case it's not so much the four lines of documentation you suggested but the idea of trying to align all the documentation (or any code) to make sense. If the "AutoFix" column isn't clear or if it's not clear that golangci-lint can format your code I think we should improve those areas.

If we want to add "Reformat code when the '--fix' option is specified." to linters that supports auto fix I think we should auto generate that since we already have that information. I also think referring to it as "AutoFix" which we do now is better because it's agnostic to flags and config and it will not need to be updated if we add new ways to format your code.

I just want to mention again that I'm not trying to avoid adding more context, I'm trying to point out alternative ways to help reduce the problem of not understanding what a linter does, if it supports formatting your code and shed more light on the auto fix feature, without being specific to a single linter.

I agree that the links I provided maybe isn't the best, what do you think about a section under the https://golangci-lint.run/usage/linters/ page that explains what the "AutoFix" column mean and add crosslinks to the configuration page? That way it's close to the linter description and the column and it will also be an improvement for all linters that supports this, not only goimports.

EDIT My somewhat long text made it sound like I really care about this change. If you don't agree with me at all and think it's better to just merge what you have now go for it. It's not the end of the world if not everything is consistent and only one linter mentions the --fix flag. I think the important bit here was to clarify what the local-prefixes config does and I think this PR solves that.

[sebastien-rosset]

As a related note, there is a proposal to make goimports more opinionated about go import formatting: golang/go#20818. There is also a project that effectively implements that proposal: https://github.com/rinchsan/gosimports.
It would be nice to have a golangci-lint linter that validates whether imports are formatted according to the proposal. Either by introducing a new gosimport linter, or by adding a setting in the existing goimports linter to specify which command to use, e.g. goimport or gosimport.

[ldez]

you can use gci

[sebastien-rosset]

you can use gci

I could add a reference to cgi from the goimports linter:

See cgi for a more opinionated go import linter.

[ldez]

Our Netlify build is broken, I'm trying to fix it, so you don't need to open/close your PR.
I know how to fix but I don't have the rights, so I'm talking with Denis to handle that.