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

ci(lint): enable nolintlint and remove redundant comments #12926

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

alexandear
Copy link
Contributor

This PR enables nolintlint linter to check //nolint comments. Also, removes redundant //nolint: directives.

Signed-off-by: Oleksandr Redko <Oleksandr_Redko@epam.com>
@bboreham
Copy link
Member

bboreham commented Oct 9, 2023

Can you explain a little bit more background why we want this linter and why the comments are redundant, please?

@alexandear
Copy link
Contributor Author

alexandear commented Oct 9, 2023

@bboreham yeah.

nolintlint is a Go static analysis tool to find ill-formed or insufficiently explained //nolint directives. In other words, it allows us to search for mistakenly added //nolint comments.

When I enabled nolintlint and run golangci-lint I got the following lint issues:

❯ ./bin/golangci-lint run
cmd/prometheus/main.go:15:1: directive `// nolint:revive // Many unsued function arguments in this file by design.` should be written without leading space as `//nolint:revive // Many unsued function arguments in this file by design.` (nolintlint)
// nolint:revive // Many unsued function arguments in this file by design.
^
cmd/promtool/backfill_test.go:47:113: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
func queryAllSeries(t testing.TB, q storage.Querier, expectedMinTime, expectedMaxTime int64) []backfillSample { // nolint:revive
                                                                                                                ^
cmd/promtool/main.go:414:1: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
// nolint:revive
^
cmd/promtool/rules_test.go:38:145: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
func (mockAPI mockQueryRangeAPI) QueryRange(_ context.Context, query string, r v1.Range, opts ...v1.Option) (model.Value, v1.Warnings, error) { // nolint:revive
                                                                                                                                                ^
discovery/kubernetes/endpoints.go:14:1: directive `// nolint:revive // Many legitimately empty blocks in this file.` should be written without leading space as `//nolint:revive // Many legitimately empty blocks in this file.` (nolintlint)
// nolint:revive // Many legitimately empty blocks in this file.
^
discovery/kubernetes/endpointslice.go:193:28: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                for e.process(ctx, ch) { // nolint:revive
                                         ^
discovery/kubernetes/ingress.go:91:28: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                for i.process(ctx, ch) { // nolint:revive
                                         ^
discovery/kubernetes/node.go:99:28: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                for n.process(ctx, ch) { // nolint:revive
                                         ^
discovery/kubernetes/pod.go:134:28: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                for p.process(ctx, ch) { // nolint:revive
                                         ^
discovery/kubernetes/service.go:94:28: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                for s.process(ctx, ch) { // nolint:revive
                                         ^
discovery/zookeeper/zookeeper.go:196:27: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                        for range pathUpdate { // nolint:revive
                                               ^
promql/engine.go:2019:49: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                for drop = 0; floats[drop].T < mint; drop++ { // nolint:revive
                                                              ^
promql/engine.go:2041:53: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                for drop = 0; histograms[drop].T < mint; drop++ { // nolint:revive
                                                                  ^
promql/engine_test.go:1659:2: directive `//nolint:govet` is unused for linter "govet" (nolintlint)
        //nolint:govet
        ^
promql/functions.go:14:1: directive `// nolint:revive // Many unsued function arguments in this file by design.` should be written without leading space as `//nolint:revive // Many unsued function arguments in this file by design.` (nolintlint)
// nolint:revive // Many unsued function arguments in this file by design.
^
promql/parser/ast.go:58:2: directive `// nolint:unused` should be written without leading space as `//nolint:unused` (nolintlint)
        // nolint:unused
        ^
promql/parser/lex.go:14:1: directive `// nolint:revive // Many legitimately empty blocks in this file.` should be written without leading space as `//nolint:revive // Many legitimately empty blocks in this file.` (nolintlint)
// nolint:revive // Many legitimately empty blocks in this file.
^
promql/parser/parse.go:75:1: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
// nolint:revive
^
promql/parser/parse.go:663:94: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                        for r.Start = n.LHS.PositionRange().End; isSpace(rune(p.lex.input[r.Start])); r.Start++ { // nolint:revive
                                                                                                                  ^
promql/parser/parse.go:665:94: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                        for r.End = n.RHS.PositionRange().Start - 1; isSpace(rune(p.lex.input[r.End])); r.End-- { // nolint:revive
                                                                                                                  ^
promql/parser/parse_test.go:4039:2: directive `//nolint:govet` is unused for linter "govet" (nolintlint)
        //nolint:govet
        ^
scrape/target.go:148:2: directive `//nolint: errcheck` is unused for linter "errcheck" (nolintlint)
        //nolint: errcheck
        ^
scrape/target.go:150:2: directive `//nolint: errcheck` is unused for linter "errcheck" (nolintlint)
        //nolint: errcheck
        ^
storage/buffer_test.go:214:38: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
        for it.Next() != chunkenc.ValNone { // nolint:revive
                                            ^
storage/memoized_iterator_test.go:115:38: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
        for it.Next() != chunkenc.ValNone { // nolint:revive
                                            ^
storage/remote/azuread/azuread.go:50:29: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
type AzureADConfig struct { // nolint:revive
                            ^
template/template.go:184:32: directive `// nolint:staticcheck` should be written without leading space as `//nolint:staticcheck` (nolintlint)
                        "title":     strings.Title, // nolint:staticcheck
                                                    ^
tsdb/chunkenc/float_histogram.go:106:39: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
        for it.Next() == ValFloatHistogram { // nolint:revive
                                             ^
tsdb/chunkenc/float_histogram_test.go:160:40: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
        for itX.Next() == ValFloatHistogram { // nolint:revive
                                              ^
tsdb/chunkenc/histogram.go:117:34: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
        for it.Next() == ValHistogram { // nolint:revive
                                        ^
tsdb/chunkenc/histogram_test.go:165:35: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
        for itX.Next() == ValHistogram { // nolint:revive
                                         ^
tsdb/chunkenc/xor.go:102:29: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
        for it.Next() != ValNone { // nolint:revive
                                   ^
tsdb/errors/errors.go:28:43: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
func NewMulti(errs ...error) multiError { // nolint:revive
                                          ^
tsdb/head_test.go:14:1: directive `// nolint:revive // Many legitimately empty blocks in this file.` should be written without leading space as `//nolint:revive // Many legitimately empty blocks in this file.` (nolintlint)
// nolint:revive // Many legitimately empty blocks in this file.
^
tsdb/head_wal.go:14:1: directive `// nolint:revive // Many legitimately empty blocks in this file.` should be written without leading space as `//nolint:revive // Many legitimately empty blocks in this file.` (nolintlint)
// nolint:revive // Many legitimately empty blocks in this file.
^
tsdb/ooo_head_read.go:14:1: directive `// nolint:revive // Many unused function arguments in this file by design.` should be written without leading space as `//nolint:revive // Many unused function arguments in this file by design.` (nolintlint)
// nolint:revive // Many unused function arguments in this file by design.
^
tsdb/querier_bench_test.go:270:22: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                                        for ss.Next() { // nolint:revive
                                                        ^
tsdb/querier_test.go:14:1: directive `// nolint:revive // Many unsued function arguments in this file by design.` should be written without leading space as `//nolint:revive // Many unsued function arguments in this file by design.` (nolintlint)
// nolint:revive // Many unsued function arguments in this file by design.
^
tsdb/wal.go:14:1: directive `// nolint:revive // Many unsued function arguments in this file by design.` should be written without leading space as `//nolint:revive // Many unsued function arguments in this file by design.` (nolintlint)
// nolint:revive // Many unsued function arguments in this file by design.
^
tsdb/wlog/reader_test.go:544:24: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                        for reader.Next() { // nolint:revive
                                            ^
tsdb/wlog/wlog.go:974:1: directive `// nolint:revive // TODO: Consider exporting segmentBufReader` should be written without leading space as `//nolint:revive // TODO: Consider exporting segmentBufReader` (nolintlint)
// nolint:revive // TODO: Consider exporting segmentBufReader
^
tsdb/wlog/wlog.go:986:1: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
// nolint:revive
^
tsdb/wlog/wlog_test.go:167:20: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                                for r.Next() { // nolint:revive
                                               ^
util/annotations/annotations.go:94:1: directive `//nolint:revive // Ignore ST1012` is unused for linter "revive" (nolintlint)
//nolint:revive // Ignore ST1012
^
util/runtime/statfs_default.go:75:2: directive `//nolint:unconvert // This ensure Type format on all Platforms` is unused for linter "unconvert" (nolintlint)
        //nolint:unconvert // This ensure Type format on all Platforms
        ^
util/treecache/treecache.go:119:30: directive `// nolint:revive` should be written without leading space as `//nolint:revive` (nolintlint)
                for range tc.head.events { // nolint:revive
                                           ^
util/zeropool/pool_test.go:152:17: directive `//nolint:staticcheck // This allocates.` is unused for linter "staticcheck" (nolintlint)
        pool.Put(item) //nolint:staticcheck // This allocates.
                       ^
util/zeropool/pool_test.go:158:18: directive `//nolint:staticcheck  // New pointer.` is unused for linter "staticcheck" (nolintlint)
                pool.Put(&buf) //nolint:staticcheck  // New pointer.
                               ^
web/api/v1/errors_test.go:14:1: directive `// nolint:revive // Many unsued function arguments in this file by design.` should be written without leading space as `//nolint:revive // Many unsued function arguments in this file by design.` (nolintlint)
// nolint:revive // Many unsued function arguments in this file by design.
^

I fixed all these issues in this PR and removed 19 unnecessary code lines.

Copy link
Member

@jesusvazquez jesusvazquez left a comment

Choose a reason for hiding this comment

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

This PR was reviewed during the Prometheus Bug Scrub event.

TIL about nolintlint and found it very cool 🎉

Thank you for the contribution.

@jesusvazquez jesusvazquez merged commit 8e5f038 into prometheus:main Oct 31, 2023
24 checks passed
@bboreham
Copy link
Member

Sorry I'm still missing why it is OK to delete the comments as you did.
Maybe we don't run the revive linter any more?

@alexandear
Copy link
Contributor Author

alexandear commented Nov 1, 2023

@bboreham you're right. There was a problem with revive's configuration. Fixed in #13068.

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 this pull request may close these issues.

None yet

3 participants