Re: [Libguestfs] [libnbd PATCH 5/7] golang: Use 'gofmt' style recommendations on manual files

On Thu, Jul 27, 2023 at 09:50:01AM -0500, Eric Blake wrote: > On Thu, Jul 27, 2023 at 01:52:00PM +0200, Laszlo Ersek wrote: >> On 7/26/23 19:50, Eric Blake wrote: >>> I ran: >>> gofmt -s -w $(git ls-files -- '**/*.go') >>> >>> then touched up a few comments in test 590 where it mis-interpreted >>> our intentions of having a single sentence that occupies more than 80 >>> columns. >>> >>> Most of the changes are whitespace fixes (consistent use of TAB, >>> altering the whitespace around operators), using preferred ordering >>> for imports, and eliding excess syntax (unneeded () or ;). >>> >>> This patch only adjusts files directly in git control. Later patches >>> will tackle (most) Go style problems in the generated files, then >>> automate a way to ensure we don't regress. >>> >>> Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; >>> --- >>> +++ b/golang/libnbd_620_stats_test.go >>> @@ -124,16 +124,16 @@ func Test620Stats(t *testing.T) { >>> t.Fatalf("%s", err) >>> } >>> >>> - if bs2 != bs1 + 28 { >>> + if bs2 != bs1+28 { >>> t.Fatalf("unexpected value for bs2") >>> } > >>> - if cr3 != cr2 + slop { >>> + if cr3 != cr2+slop { >>> t.Fatalf("unexpected value for cr3") >>> } >>> } >> >> I've done nearly nothing in Go, and generally the updates look >> justified... but the changes to "libnbd_620_stats_test.go" are hideous. >> Who on Earth considers >> >> if cr3 != cr2+slop >> >> superior to >> >> if cr3 != cr2 + slop >> >> ??? >> >> *shudder* > > That's gofmt for you. I was surprised by it too. > >> >> Question in passing: if we wrote >> >> if cr3 != (cr2 + slop) >> >> would gofmt contract that too to >> >> if cr3 != (cr2+slop) > > Equally surprising, gofmt (at least from golang-bin-1.20.6-1.fc38) > does NOT remove the spaces from that format, nor does it complain that > the () are spurious. Of course, future gofmt may change style again, > but now we have a regression test in place to catch that. And we may > be faced with decisions down the road on how to pacify cases where > gofmt itself changes styles from one release of the language to the > next (if that happens, that would be a strong argument that we should > remove gofmt from a gating 'make check' test, and instead incorporate > it as an optional 'make dist' step - if we reach a point where there > is no longer "the" canonical formatting, we shouls still favor > tarballs created with "a" canoncial format). snip > as well as amending the commit message to mention the manual touchup > needed to work around the (in our opinion, misguided) advice from Go. IMHO that is defeating the point of using 'go fmt'. The value of 'go fmt' is that developers & projects no longer need to endlessly debate style. There is a consistent format across every project in the world that is written in Go. The value of this consistency outweighs the negatives of the few cases where formatting isn't the preference of individual developers. People will usually have their editors set to auto run 'go fmt' on any '.go' file, and telling them to manually alter certain style rules the project didn't like is an unecessary burden.