Skip to content

fix: linter errors #276

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
niemeyer merged 13 commits into from
Apr 27, 2026
Merged

fix: linter errors #276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3952231
lint: respect QF1008
upils Mar 16, 2026
fd09913
lint: respect QF1004
upils Mar 16, 2026
fe8559d
lint: respect QF1001
upils Mar 16, 2026
76e9807
lint: respect S1005
upils Mar 16, 2026
76978ba
lint: respect S1011
upils Mar 16, 2026
dc1a6ff
lint: respect ST1005
upils Mar 16, 2026
e26fad1
lint: respect ST1003
upils Mar 16, 2026
5923e1a
lint: respect QF1009
upils Mar 16, 2026
4d8ac23
lint: respect QF1012
upils Mar 16, 2026
dca09c5
lint: respect ST1012
upils Mar 16, 2026
881f312
lint: respect QF1011
upils Mar 16, 2026
122c3c4
lint: remove unused exclusion
upils Mar 16, 2026
444110c
lint: formatting
upils Mar 16, 2026
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
43 changes: 20 additions & 23 deletions .golangci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,6 @@ linters:
- errcheck
path: internal/setup/fetch.go
text: lockFile.Unlock.*not checked
- linters:
- unused
path: cmd/chisel/main.go
text: addDebugCommand.*unused
- linters:
- unused
path: ^.*/log.go$
Expand All @@ -50,25 +46,26 @@ linters:
- staticcheck
path: ^.*.go$
text: '"golang.org/x/crypto/openpgp/\w+" is deprecated'
settings:
staticcheck:
checks:
- all
# Checks now detecting new issues after migrating to golangci-lint v2.
# TODO: Remove these and solve raised issues.
- -QF1008
- -ST1005
- -ST1012
- -QF1004
- -ST1011
- -ST1005
- -ST1003
- -QF1009
- -QF1011
- -S1011
- -S1005
- -QF1001
- -QF1012
- linters:
- staticcheck
path: cmd/chisel/.*.go
text: "ST1005:"
- linters:
- staticcheck
path: ^.*/log.go$
text: "ST1003:"
- linters:
- staticcheck
path: internal/cache/cache.go
text: "ST1012: error var MissErr"
- linters:
- staticcheck
path: internal/setup/setup.go
text: "ST1012: error var preferNone"
Comment on lines +60 to +64
Copy link
Copy Markdown
Collaborator

@letFunny letFunny Apr 23, 2026

Choose a reason for hiding this comment

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

Some of these like ST1012 or ST1005 are too opinionated for my taste. Why not disable them in general as you had done before? I don't want to write a new error that is not errSomething and see a linter error, while the existing ones work. If we think the rule is invalid or too opinionated then we should block it directly, not only allow current usages.

Copy link
Copy Markdown
Collaborator Author

@upils upils Apr 24, 2026

Choose a reason for hiding this comment

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

My goal here was to:

  • not force us to rename existing errors not respecting the rule (especially because MissErr is exported)
  • but also prevent us from introducing more errors not respecting it.

ST1012 enforces the naming convention recommended in https://go.dev/wiki/Errors#naming, so it does not seem too opinionated to me (at least not more than other idiomatic rules of the language). I even think preferNone could be renamed to respect the rule.

Regarding ST1005: we are already respecting the rule (also implicitly respected in examples of PL007), and this linter rule should help us catch any deviation. I added an exception for cmd/chisel/*.go because the CLI prints errors that require final punctuation to be nice messages to users, so I think the exception is fine here (I am not 100% happy about the granularity here, but this is a limitation due to not using inline comments for exceptions).

Copy link
Copy Markdown
Collaborator

@letFunny letFunny Apr 24, 2026

Choose a reason for hiding this comment

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

If you think current exceptions could be renamed then okay and you think the rule is beneficial. What I wouldn't like happening is to add more exceptions to the rule in the future.

Copy link
Copy Markdown
Collaborator Author

@upils upils Apr 24, 2026

Choose a reason for hiding this comment

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

I will add a TODO to mention no exception should be added for these rules and existing ones should be removed when possible.

Copy link
Copy Markdown
Contributor

@niemeyer niemeyer Apr 27, 2026

Choose a reason for hiding this comment

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

Why not fixing these right now? It's just two cases.

Copy link
Copy Markdown
Collaborator Author

@upils upils Apr 27, 2026

Choose a reason for hiding this comment

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

I expected the renaming of these errors to be a bit controversial because the unexported one was very carefully named and the other one is exported. So I did not want this to block the bulk of the cleaning. I prepared #293 to address it.

- linters:
- staticcheck
path: internal/testutil/containschecker.go
text: "QF1011"
issues:
max-issues-per-linter: 0
max-same-issues: 0
8 changes: 4 additions & 4 deletions cmd/chisel/cmd_help.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func addHelp(parser *flags.Parser) error {
// case, parser.Command.Active should be the command
// on which help is being requested (like "chisel foo
// --help", active is foo), or nil in the toplevel.
if parser.Command.Active == nil {
if parser.Active == nil {
// this means *either* a bare 'chisel --help',
// *or* 'chisel --help command'
//
Expand Down Expand Up @@ -101,7 +101,7 @@ func (w *manfixer) Write(buf []byte) (int, error) {
var tpRegexp = regexp.MustCompile(`(?m)(?:^\.TP\n)+`)

func (w *manfixer) flush() error {
str := tpRegexp.ReplaceAllLiteralString(w.Buffer.String(), ".TP\n")
str := tpRegexp.ReplaceAllLiteralString(w.String(), ".TP\n")
_, err := io.Copy(Stdout, strings.NewReader(str))
return err
}
Expand Down Expand Up @@ -131,12 +131,12 @@ func (cmd cmdHelp) Execute(args []string) error {
// the subcommand is set below. The Active command at this point is `chisel
// help`, in the loop below we change it to be `chisel os.Args[1]
// os.Args[2] ...`.
cmd.parser.Command.Active = nil
cmd.parser.Active = nil
for _, subname := range cmd.Positional.Subs {
subcmd = subcmd.Find(subname)
if subcmd == nil {
sug := "chisel help"
if x := cmd.parser.Command.Active; x != nil && x.Name != "help" {
if x := cmd.parser.Active; x != nil && x.Name != "help" {
sug = "chisel help " + x.Name
}
return fmt.Errorf("unknown command %q, see '%s'.", subname, sug)
Expand Down
6 changes: 3 additions & 3 deletions cmd/chisel/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ func Parser() *flags.Parser {
name = string(opt.ShortName)
}
desc, ok := c.optDescs[name]
if !(c.optDescs == nil || ok) {
Copy link
Copy Markdown
Collaborator

@letFunny letFunny Apr 23, 2026

Choose a reason for hiding this comment

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

I am not sure how Gustavo feels about this linter. In general I would refrain from re-writing boolean conditions by negating them. While it is true that the result is the same, I think the readability is not the same, and IMO we should respect the author's decision to write it this same if it is more clear / more uniform.

Copy link
Copy Markdown
Collaborator Author

@upils upils Apr 24, 2026

Choose a reason for hiding this comment

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

I see your point and I am on the fence on this one.

On one hand, I agree that we might have cases where a form could be more readable or better convey the author's intention. So being forced by the linter might be impractical.

On the other hand, we have more than 150 occurrences of if with && and/or || statements, and the linter only found 2 cases not respecting the rule. Also, maybe I am missing something but for these examples I happen to find the rework statements easier to reason about. So it seems we are already following this rule "naturally". In that case we might as well make it automatically verified and properly exclude and document cases where the form matters.

WDYT?

Copy link
Copy Markdown
Collaborator

@letFunny letFunny Apr 24, 2026

Choose a reason for hiding this comment

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

In these two cases I agree as well, but I find it stylistic more than anything else. I can see how someone prefers the negation. Anyway, no strong opinion on my side, we can try it out and remove if we are not happy.

Copy link
Copy Markdown
Contributor

@niemeyer niemeyer Apr 27, 2026

Choose a reason for hiding this comment

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

Yeah, Alberto has a good point, but this particular case seems like the linter did a good job. The replacement is more readable than the original, and it actually aligns with similar states before and after this one. So +1 on the linter result.

if c.optDescs != nil && !ok {
panicf("%s missing description for %s", c.name, name)
}
lintDesc(c.name, name, desc, opt.Description)
Expand Down Expand Up @@ -261,7 +261,7 @@ func Parser() *flags.Parser {
name = string(opt.ShortName)
}
desc, ok := c.optDescs[name]
if !(c.optDescs == nil || ok) {
if c.optDescs != nil && !ok {
panicf("%s missing description for %s", c.name, name)
}
lintDesc(c.name, name, desc, opt.Description)
Expand Down Expand Up @@ -345,7 +345,7 @@ func run() error {
sug := "chisel help"
if len(xtra) > 0 {
sub = xtra[0]
if x := parser.Command.Active; x != nil && x.Name != "help" {
if x := parser.Active; x != nil && x.Name != "help" {
sug = "chisel help " + x.Name
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/archive/testarchive/testarchive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func (r *Release) Content() []byte {
digests := bytes.Buffer{}
for _, item := range r.Items {
content := item.Content()
digests.WriteString(fmt.Sprintf(" %s %d %s\n", makeSha256(content), len(content), item.Path()))
fmt.Fprintf(&digests, " %s %d %s\n", makeSha256(content), len(content), item.Path())
Copy link
Copy Markdown
Collaborator

@letFunny letFunny Apr 23, 2026

Choose a reason for hiding this comment

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

Interesting that this is automatic, how was this suggested? I checked the API and it appears to be the same minus a possibly different error returned, but we are not using it anyway...

Copy link
Copy Markdown
Collaborator Author

@upils upils Apr 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This is rule QF1012 from staticcheck (see https://go.dev/gopls/analyzers#qf1012-use-fmtfprintfx--instead-of-xwritefmtsprintf). The rationale is partially explained here and this may even be integrated in go vet/go fix at some point, see golang/go#76918.

I also agree that it is a relatively safe change since we are not doing anything with the returned values.

Copy link
Copy Markdown
Collaborator

@letFunny letFunny Apr 24, 2026

Choose a reason for hiding this comment

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

Interesting, let's keep it then

}
content := fmt.Sprintf(string(testutil.Reindent(`
Origin: Ubuntu
Expand Down
2 changes: 1 addition & 1 deletion internal/deb/extract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ func extractHardLinks(pkgReader io.ReadSeeker, opts *extractHardLinkOptions) err
// this package.
if len(opts.pendingLinks) > 0 {
var targets []string
for target, _ := range opts.pendingLinks {
for target := range opts.pendingLinks {
targets = append(targets, target)
}
sort.Strings(targets)
Expand Down
9 changes: 3 additions & 6 deletions internal/slicer/slicer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1990,7 +1990,7 @@ func (s *S) TestRun(c *C) {
m := make(map[string]string)
for k, v := range t.release {
if !strings.Contains(v, "v2-archives:") {
v = strings.Replace(v, "archives:", "v2-archives:", -1)
v = strings.ReplaceAll(v, "archives:", "v2-archives:")
}
m[k] = v
}
Expand All @@ -2007,7 +2007,7 @@ func (s *S) TestRun(c *C) {
if strings.Contains(v, "format: v1") &&
!strings.Contains(v, "v2-archives:") &&
!strings.Contains(v, "default: true") {
v = strings.Replace(v, "format: v1", "format: v2", -1)
v = strings.ReplaceAll(v, "format: v1", "format: v2")
}
m[k] = v
}
Expand Down Expand Up @@ -2190,10 +2190,7 @@ func treeDumpManifestPaths(mfest *manifest.Manifest) (map[string]string, error)
}

// append {slice1, ..., sliceN} to the end of the path dump.
slicesStr := make([]string, 0, len(path.Slices))
for _, slice := range path.Slices {
slicesStr = append(slicesStr, slice)
}
slicesStr := slices.Clone(path.Slices)
sort.Strings(slicesStr)
result[path.Path] = fmt.Sprintf("%s {%s}", fsDump, strings.Join(slicesStr, ","))
return nil
Expand Down
2 changes: 1 addition & 1 deletion internal/testutil/pkgdata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func fixupTarEntry(entry *TarEntry) {
if hdr.Gid == 0 && hdr.Gname == "" {
hdr.Gname = "root"
}
if hdr.ModTime == zeroTime {
if hdr.ModTime.Equal(zeroTime) {
hdr.ModTime = epochStartTime
}
if hdr.Format == 0 {
Expand Down
Loading