fix: linter errors#276
Conversation
letFunny
left a comment
letFunny
left a comment
There was a problem hiding this comment.
Thanks for looking into this Paul. Let's monitor this closely and see if these become a pain and then disable the checks that are too opinionated.
A couple of minor comments but I don't want to block the PR.
| name = string(opt.ShortName) | ||
| } | ||
| desc, ok := c.optDescs[name] | ||
| if !(c.optDescs == nil || ok) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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()) |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Interesting, let's keep it then
| text: "ST1012: error var MissErr" | ||
| - linters: | ||
| - staticcheck | ||
| path: internal/setup/setup.go | ||
| text: "ST1012: error var preferNone" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I will add a TODO to mention no exception should be added for these rules and existing ones should be removed when possible.
There was a problem hiding this comment.
Why not fixing these right now? It's just two cases.
There was a problem hiding this comment.
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.
letFunny
left a comment
letFunny
left a comment
There was a problem hiding this comment.
Looks good to me. There are only a couple of minor things that I think need a bit more discussion with Gustavo. I already wrote my opinion but it is not a strong opinion that blocks the PR, I am more than okay with trying this out and see if it because tedious or not.
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
A comment and an open question. I'm merging this right away since there isn't a blocker. We can have a follow up if necessary.
| name = string(opt.ShortName) | ||
| } | ||
| desc, ok := c.optDescs[name] | ||
| if !(c.optDescs == nil || ok) { |
There was a problem hiding this comment.
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.
| text: "ST1012: error var MissErr" | ||
| - linters: | ||
| - staticcheck | ||
| path: internal/setup/setup.go | ||
| text: "ST1012: error var preferNone" |
There was a problem hiding this comment.
Why not fixing these right now? It's just two cases.
|
4 participants
Reenable linter rules and fix associated errors excluded in #274.
Following the existing approach, exclusions were added to the golangci-lint
configuration instead of
//nolintdirectives in the code. This could lead tomissing true positives as the exclusion can sometimes be unprecise.
Also remove a now useless exclusion.