lint: replace gocyclo with gocognit - #14055
Closed
ndeloof wants to merge 3 commits into
Closed
Conversation
Cognitive complexity (gocognit, threshold 30 — the linter default) fits this codebase better than raw cyclomatic complexity: it barely charges guard clauses and early returns, and penalizes nesting instead. As a result 8 of the 21 //nolint:gocyclo suppressions become unnecessary, while deeply nested functions gocyclo never flagged are now caught. The 19 functions still above the threshold keep a suppression, each marked FIXME to complete the migration by restructuring them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR is a clean mechanical replacement of gocyclo with gocognit in the linter configuration. The changes are well-structured:
.golangci.yml:gocognitcorrectly enabled withmin-complexity: 30(the linter default);gocyclofully removed.- 8 suppressions removed from functions whose gocognit score is now below 30 — consistent with gocognit's guard-clause-friendly scoring that penalizes nesting over linear branching.
- 19 suppressions retained (converted from
//nolint:gocycloto//nolint:gocognit) each with aFIXMEfor future refactoring — count matches the PR description. - Nolint directive placement: All
//nolint:gocognitdirectives appear immediately before their respectivefuncdeclarations with no intervening blank lines, satisfying golangci-lint's directive-recognition rules.
No bugs introduced by this PR.
thaJeztah
previously approved these changes
Aug 17, 2026
thaJeztah
left a comment
thaJeztah
left a comment
Member
There was a problem hiding this comment.
LGTM, nice!
Does golangci have an explicit "disable" section as well? This may be a case where putting gocyclo as explicitly disabled with a short comment could prevent someone adding it back in future.
doBuildBake mixed five concerns in a 300-line body (cognitive complexity 88): progress display setup, translation of the project into a bake file definition, temp metadata file allocation, bake command construction, stderr rawjson streaming, and result collection. Each now lives in its own function; the driver reads as the sequence of those stages (cognitive complexity 17), and the FIXME suppression is gone. The bake variable was named to avoid shadowing the docker/cli 'build' package. No behavior change: prepareBakeBuild emits the same config, and the stderr loop keeps the decoder-per-line semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
gocognit replaced gocyclo on purpose: raw cyclomatic complexity charges a flat guard clause the same as a deeply nested branch, so our fail-fast style accumulated //nolint suppressions on functions that are long but flat. Cognitive complexity penalizes nesting and barely charges early returns, which keeps the linter's signal on genuinely tangled code instead. With 'default: none' the disable entry is functionally redundant, but it records that decision exactly where someone would re-add the linter, with a hard stop: golangci-lint rejects a config listing the same linter in both enable and disable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Contributor
Author
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
Replaced the
gocyclolinter withgocognit(cognitive complexity, threshold 30 — the linter default). Cognitive complexity barely charges guard clauses and early returns, and penalizes nesting instead, which matches how this codebase is written: 8 of the 21//nolint:gocyclosuppressions become unnecessary, while deeply nested functions gocyclo never flagged are now caught. The 19 functions still above the threshold keep a suppression, each markedFIXMEso they can be restructured over time to complete the migration.includes refactoring of doBuildBake (hot spot after migration to gocognit, never identified by gocyclo)
Related issue
n/a
🤖 Generated with Claude Code