Linter and fixes

[horejsek]

Adding linter config with build job for it, and fixes to pass linter. Some linters are still disabled because there is too many issues with low additional value. Changes includes also some fixes, for example, Parse in commands/subscribe.go or commands/uid.go. Hopefully it will help to reduce bugs, notice and fix unchecked errors, use recommendation to prealloc to speed up stuff, and improve code in general.

[codecov[bot]]

Codecov Report

Merging #370 into master will increase coverage by 0.02%. The diff coverage is 66.37%.

@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
+ Coverage   68.89%   68.92%   +0.02%     
==========================================
  Files          41       41              
  Lines        3797     3794       -3     
==========================================
- Hits         2616     2615       -1     
  Misses        882      882              
+ Partials      299      297       -2     

Impacted Files	Coverage Δ
client/cmd_auth.go	63.07% <ø> (ø)
client/cmd_noauth.go	67.05% <ø> (ø)
command.go	60.00% <ø> (ø)
conn.go	52.52% <ø> (ø)
date.go	100.00% <ø> (ø)
imap.go	0.00% <0.00%> (ø)
responses/authenticate.go	0.00% <0.00%> (ø)
responses/capability.go	0.00% <ø> (ø)
responses/expunge.go	0.00% <ø> (ø)
responses/fetch.go	0.00% <ø> (ø)
... and 25 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 011063d...465f5d3. Read the comment docs.

[foxcpp]

IMO Most linters report potential problems, not problems. I believe hard-failing build because of linter warnings is a little bit unnecessary. I think we had this conversation with @emersion in the context of maddy.

[emersion]

Yeah, I'd prefer not to add the linter to CI, and not to clutter the codebase with nolint comments.

[horejsek]

If linter is not in CI, it will not be used. As said, linter provides potential problems, but no one would check it then. Not mentioning if there would be no nolint comments, because there would be a lot of noise in the lint output. Having not lint in CI, or not clearing OK stuff, simply means lint is useless. If you don't want it, well, feel free to close this merge request. But note that lint found real bugs and many unchecked errors (which still need to be handled). It would be sad to not use it, moreover in static-typed language where lint can help a lot.

[foxcpp]

As an alternative to throwing nolint all over the place it is probably a good idea to eventually resolve minor 'issues' to reduce noise even if it is just documentation formatting for example.

[horejsek]

I agree it's better to solve all issues. I disabled a lot of linters which may be good to turn on one day, too, to improve documentation (e.g., comments). I left nolint comments on places such as Uid -> UID, prealloc opportunities (I fixed some, but some places are harder to do), aligned structs better, and so on which would change interface and make breaking change. From my point of view, many nolint comments here are actually TODOs for v2. Then there are some nolint comments which could be solved without introducing breaking change right away but it would take a lot of time so they are like TODOs for someone changing those particular parts to improve those lines as well.

[emersion]

As said, linter provides potential problems, but no one would check it then.

Linters have false-positives. I think it's great if someone takes the time to carefully read the linter output and send patches, but I don't think we should enforce them.

Note, the CI should already run go vet as part of go test.

prealloc opportunities (I fixed some, but some places are harder to do), aligned structs better

TBH I'm not a fan of these. I prefer increased readability instead of micro-optimizations.

[horejsek]

I think we will not agree here, but that's fine. What is the problem is that I don't understand what you want, then. Lint in CI as warning only, no lint in CI, or not accepting this MR at all? I think it would be good to have at least in warning mode like coverage.

[emersion]

Lint in CI as warning only

I think I'd be fine with that. Should probably be in a completely separate build job.

no lint in CI

That's the current status quo. I'm fine with this too.

not accepting this MR at all

I'm fine with merging fixes for issues found via linters ofc. However getting go-imap in a state where linters don't report any warning isn't a goal, and all issues aren't necessarily worth fixing (e.g. I'd prefer no pre-allocation over very elaborate and complicated pre-allocation).

[foxcpp]

Data point: Over time I managed to gradually reduce the amount of warnings reported by linters for maddy code base but that sure was a long way from multiple full screens of warnings to a small amount of minor nits. Given that, there are only 2 nolint comments in over 30kLoC:.

I believe there is a point in doing so. Large amount of warnings reported by linters may make it harder to see warnings that correspond to actual problems and once we get there it is much easier to keep it clean.

[horejsek]

I don't know how to configure build to be warning only or how to make other build. I solved it by ignoring errors. Not great, people need to open the log to see the lint issues, but at least something. I also disabled prealloc and maligned linters completely and removed particular nolint comments. Hope it's fine now.

[horejsek]

So can this be merged? :-)

[emersion]

Closing because this is superseded by go-imap v2.