Skip to content

doc: improve documentation for goimport local-prefixes #3349

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

Merged
merged 15 commits into from
Dec 13, 2022
Merged

doc: improve documentation for goimport local-prefixes #3349

Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d61f629
Add better documentation for goimport local-prefixes
sebastien-rosset Nov 7, 2022
349d563
Merge branch 'master' into importdocimprovement
sebastien-rosset Nov 7, 2022
b13722b
Add better documentation for goimport local-prefixes
sebastien-rosset Nov 7, 2022
4ce3c0e
Merge branch 'importdocimprovement' of github.com:CiscoM31/golangci-l…
sebastien-rosset Nov 7, 2022
e4a50f5
Improve documentation of 'goimports' linter
sebastien-rosset Nov 7, 2022
b6a52e2
Improve documentation of 'goimports' linter
sebastien-rosset Nov 7, 2022
507450d
Improve documentation
sebastien-rosset Nov 7, 2022
7e14c3a
Improve documentation
sebastien-rosset Nov 7, 2022
e0bb080
Merge branch 'master' into importdocimprovement
sebastien-rosset Nov 7, 2022
19204f9
improve documentation
sebastien-rosset Nov 9, 2022
560e0b0
Merge branch 'importdocimprovement' of github.com:CiscoM31/golangci-l…
sebastien-rosset Nov 9, 2022
aa9161d
Merge branch 'master' into importdocimprovement
sebastien-rosset Nov 10, 2022
a5f7046
Merge branch 'master' into importdocimprovement
sebastien-rosset Nov 15, 2022
58e9fcd
Merge branch 'master' into importdocimprovement
sebastien-rosset Dec 1, 2022
eb5f5db
Merge branch 'master' into importdocimprovement
sebastien-rosset Dec 5, 2022
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
5 changes: 4 additions & 1 deletion .golangci.reference.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,10 @@ linters-settings:

goimports:
# Put imports beginning with prefix after 3rd-party packages.
# It's a comma-separated list of prefixes.
# It's a comma-separated list of prefixes, which, if set, instructs to sort the import paths
# with the given prefixes into another group after 3rd-party packages.
# If imports are not grouped by local-prefixes, a linter issue is created with a message
# indicating the file is not `goimports`-ed with -local ... (goimports).
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

# Default: ""
local-prefixes: github.com/org/project

Expand Down