These all look good and there are no semantic changes. Have built and
run myself and everything works as expected.

Acked-by: Daniel Patterson <me@danielpatterson.dev>

On Sun, 6 Mar 2022, at 18:20, inwit wrote:
> now that error strings are brought up, wouldn't it make sense to give
> the logs a bit of more sense prepending errors with a common string
> like "ERROR: "? Wouldn't that make it easier to spot problems for the
> untrained eye?

Logs categorised by importance is definitely something I would like to
see brought to the codebase at some point in the future, but I think
it would be best to introduce it alongside some structured logging
package like zig or zerolog. I don't think it is within the scope of
this patchset.

Daniel

On Sun, 6 Mar 2022, at 18:28, Moritz Poldrack wrote:
> The main reason for that rule (ST1005) is so logged errors don't have
> random uppercase characters in them. I personally would consider the
> entire ST class of "errors" to be matters of personal taste and
> therefore should be disabled.

I feel that "personal taste" shouldn't really be a consideration when
talking about code style, consistency is much more important. I think
some individual ST checks are probably not worthwhile when considering
the effort/benefit trade-off, but I'm sure that most are.

Errors especially are an area where consistency is extremely important,
and I think if we're running into problems with capital letters in error
messages then perhaps we're using error messages wrong.

On Sun, 6 Mar 2022, at 17:40, Moritz Poldrack wrote:
> What rules have you applied and what have you set to non-critical? (so
> they don't stop the Pipeline from working) This might be a great
> moment to discuss what rules to keep, what rules to disable and what
> to consider "non-critical".

I just ran `staticcheck ./...` and `go vet ./...` and fixed some random
ones that Goland pointed out. When I write programs myself I try to
adhere to all staticcheck checks except ST1000 so that would be my vote.
Tbh I haven't looked into every single in go vet or staticcheck yet,
but I'm a big fan of enforcing pretty strict rules on a codebase.

Another useful analyser is errcheck, which we can use to find all
unhandled errors.

Note that the automated build fails because this patch is based on
Moritz's submission:
https://lists.sr.ht/~rjarry/aerc-devel/patches/30043

This commit fixes an assortment of code issues flagged up by go vet,
staticcheck, or Goland.

Signed-off-by: Daniel Patterson <me@danielpatterson.dev>
---

These fixes don't fit neatly into any of the other patches so I have
grouped them in this miscellaneous one.

 aerc.go                         |  4 ++--
 commands/account/next-folder.go |  2 +-
 commands/account/next.go        |  2 +-
 commands/account/select.go      |  2 +-
 commands/choose.go              |  2 +-
[message trimmed]

This patch adjusts identifiers to remove instances of stuttering (ie.
a type in package foo called FooBar would be foo.Foobar), instances of
variables clobbering imports (eg. foo := foo.Bar), and cases where
receivers were named inconsistently (eg func (foo Foo) x() and
func (f Foo) y() should have the same receiver name).

Signed-off-by: Daniel Patterson <me@danielpatterson.dev>
---

These are common conventions in Go code and I think they are ones we
ought to adhere to as far as is possible.

 aerc.go                       |  6 ++--
 commands/account/account.go   |  8 +++---
[message trimmed]

Structs should use keyed fields when using literals, this makes code
resiliant to changes in the underlying struct and improves readability.

Signed-off-by: Daniel Patterson <me@danielpatterson.dev>
---

This will clobber a lot of git blame, and it makes the code contain a
fair amount of repetition, but go vet doesn't like it and personally I'm
inclined to agree. Although it requires more typing, I think that
including struct fields makes code much clearer and I think it's worth
the cost.

Not to mention it also prevents issues if the struct fields get
reordered.
[message trimmed]

According to [1], error strings should not start with capital letters
(unless it is a proper noun). This patch makes all error messages start
with a lowercase letter.

[1]: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Signed-off-by: Daniel Patterson <me@danielpatterson.dev>
---

Honestly I'm not happy with this patch in its current form and I'm
mainly submitting it for feedback. It appears that we often use errors
for returning user-visible messaging when commands are used incorrectly.

Personally, I don't think this is the correct approach and I would
[message trimmed]

This patchset contains changes which resolve the vast majority of issues
from `go vet` and staticcheck, as well as fixing some inspections from
Goland's inbuilt inspections which I think are appropriate.

After this patchset is applied, there are still a few issues flagged up,
but they are all either from works in progress, or from parts of the
code I don't know well enough to address.

Depending on the response to this patchset, I may continue to work on
various refactorings of the codebase. Currently Goland points out ~200
locations where error return values are not handled, and that would
probably be where I would go next.

Another place in need of improvement is in comments on exported