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

feat: add sort-order option #4467

Merged
merged 3 commits into from
Mar 8, 2024
Merged

feat: add sort-order option #4467

merged 3 commits into from
Mar 8, 2024

Conversation

ldez
Copy link
Member

@ldez ldez commented Mar 8, 2024

Allows to change the sort order of the results.

This PR allows sorting by linter name, severity, and file information (file path, line, and column).

I still don't really understand the need behind this, but I found a "generic" way to do it, so I did it 🤷

By "generic" I mean an option not specific (like --sort-results-by-linter) and which can evolve without breaking changes (we can add more order types).

Demo

No sorting:

$ go run ./cmd/golangci-lint run --out-format=tab               
pkg/lint/linter/linter.go:44:19      revive          unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
pkg/config/output.go:61:1            gochecknoinits  don't use `init` function
pkg/config/output.go:62:2            gosimple        S1039: unnecessary use of fmt.Sprintf
pkg/result/processors/fixer.go:21:1  gochecknoinits  don't use `init` function
pkg/result/processors/fixer.go:22:2  gosimple        S1039: unnecessary use of fmt.Sprintf
exit status 1

Sort only based on file (default):

$ go run ./cmd/golangci-lint run --out-format=tab --sort-results   
pkg/config/output.go:61:1            gochecknoinits  don't use `init` function
pkg/config/output.go:62:2            gosimple        S1039: unnecessary use of fmt.Sprintf
pkg/lint/linter/linter.go:44:19      revive          unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
pkg/result/processors/fixer.go:21:1  gochecknoinits  don't use `init` function
pkg/result/processors/fixer.go:22:2  gosimple        S1039: unnecessary use of fmt.Sprintf
exit status 1

Sort only based on linter name:

$ go run ./cmd/golangci-lint run --out-format=tab --sort-order=linter --sort-results
pkg/config/output.go:61:1            gochecknoinits  don't use `init` function
pkg/result/processors/fixer.go:21:1  gochecknoinits  don't use `init` function
pkg/config/output.go:62:2            gosimple        S1039: unnecessary use of fmt.Sprintf
pkg/result/processors/fixer.go:22:2  gosimple        S1039: unnecessary use of fmt.Sprintf
pkg/lint/linter/linter.go:44:19      revive          unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
exit status 1

Fixes #4197

@ldez ldez added enhancement New feature or improvement area: output Related to issue output labels Mar 8, 2024
@ldez ldez force-pushed the feat/sort-order branch 7 times, most recently from 30105a5 to eaf0a95 Compare March 8, 2024 03:54
.golangci.reference.yml Outdated Show resolved Hide resolved
Co-authored-by: Oleksandr Redko <oleksandr.red+github@gmail.com>
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/result/processors/sort_results.go Show resolved Hide resolved
pkg/result/processors/sort_results.go Outdated Show resolved Hide resolved
@ldez ldez added this to the next milestone Mar 8, 2024
@ldez ldez merged commit 0683d45 into golangci:master Mar 8, 2024
12 checks passed
@ldez ldez deleted the feat/sort-order branch March 8, 2024 21:06
Copy link
Contributor

@Antonboom Antonboom left a comment

Choose a reason for hiding this comment

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

🫡

.golangci.reference.yml Show resolved Hide resolved
pkg/config/output.go Show resolved Hide resolved
pkg/result/processors/sort_results.go Show resolved Hide resolved
pkg/result/processors/sort_results.go Show resolved Hide resolved
pkg/result/processors/sort_results.go Show resolved Hide resolved
pkg/result/processors/sort_results.go Show resolved Hide resolved
pkg/result/processors/sort_results.go Show resolved Hide resolved
pkg/result/processors/sort_results.go Show resolved Hide resolved
pkg/result/processors/sort_results.go Show resolved Hide resolved
@dackroyd
Copy link

dackroyd commented Apr 2, 2024

I still don't really understand the need behind this, but I found a "generic" way to do it, so I did it 🤷

Thank you for adding this 🙏 Being able to sort by more than just file is helpful when uniq-by-file: false has been set (to ensure all issues are reported). I suspect that its of value even without that, as the same issue would be reported for each run, rather than it changing between runs when there are multiple on the same line.

With a stable sort order, its easier to assess changes to reported issues between versions, configuration changes etc by performing a simple diff between reports. Without this, a significant number of issues are out of order between reports, making for a lot of noise in assessing the change.

@ldez
Copy link
Member Author

ldez commented Apr 2, 2024

I'm happy if you are happy about this feature ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: output Related to issue output enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort by linter and not just line number
5 participants