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

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 at redhat.com>
> > ---
> > +++ 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).

For now, I'm happy to squash in:

diff --git i/golang/libnbd_620_stats_test.go w/golang/libnbd_620_stats_test.go
index 63667160..6cc3cdc1 100644
--- i/golang/libnbd_620_stats_test.go
+++ w/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 cs2 != cs1+1 {
+	if cs2 != (cs1 + 1) {
 		t.Fatalf("unexpected value for cs2")
 	}
-	if br2 != br1+16 { /* assumes nbdkit uses simple reply */
+	if br2 != (br1 + 16) { /* assumes nbdkit uses simple reply */
 		t.Fatalf("unexpected value for br2")
 	}
-	if cr2 != cr1+1 {
+	if cr2 != (cr1 + 1) {
 		t.Fatalf("unexpected value for cr2")
 	}

@@ -169,13 +169,13 @@ func Test620Stats(t *testing.T) {
 	if bs3 <= bs2 {
 		t.Fatalf("unexpected value for bs3")
 	}
-	if cs3 != cs2+1 {
+	if cs3 != (cs2 + 1) {
 		t.Fatalf("unexpected value for cs3")
 	}
 	if br3 < br2 {
 		t.Fatalf("unexpected value for br3")
 	}
-	if cr3 != cr2+slop {
+	if cr3 != (cr2 + slop) {
 		t.Fatalf("unexpected value for cr3")
 	}
 }

as well as amending the commit message to mention the manual touchup
needed to work around the (in our opinion, misguided) advice from Go.
> 
> ?
> 
> Asking because I suspect that gofmt's purpose with the whitespace
> removal around "+" is to emphasize that "+" binds more
strongly than
> "!=". And, if we forced the binding with () around +, then gofmt
might
> not feel the need to emphasize the difference between the binding
strengths.
> 
> Acked-by: Laszlo Ersek <lersek at redhat.com>
> 
> Laszlo
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

On 7/27/23 16:50, 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 at redhat.com>
>>> ---
>>> +++ 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).
> 
> For now, I'm happy to squash in:
> 
> diff --git i/golang/libnbd_620_stats_test.go
w/golang/libnbd_620_stats_test.go
> index 63667160..6cc3cdc1 100644
> --- i/golang/libnbd_620_stats_test.go
> +++ w/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 cs2 != cs1+1 {
> +	if cs2 != (cs1 + 1) {
>  		t.Fatalf("unexpected value for cs2")
>  	}
> -	if br2 != br1+16 { /* assumes nbdkit uses simple reply */
> +	if br2 != (br1 + 16) { /* assumes nbdkit uses simple reply */
>  		t.Fatalf("unexpected value for br2")
>  	}
> -	if cr2 != cr1+1 {
> +	if cr2 != (cr1 + 1) {
>  		t.Fatalf("unexpected value for cr2")
>  	}
> 
> @@ -169,13 +169,13 @@ func Test620Stats(t *testing.T) {
>  	if bs3 <= bs2 {
>  		t.Fatalf("unexpected value for bs3")
>  	}
> -	if cs3 != cs2+1 {
> +	if cs3 != (cs2 + 1) {
>  		t.Fatalf("unexpected value for cs3")
>  	}
>  	if br3 < br2 {
>  		t.Fatalf("unexpected value for br3")
>  	}
> -	if cr3 != cr2+slop {
> +	if cr3 != (cr2 + slop) {
>  		t.Fatalf("unexpected value for cr3")
>  	}
>  }
> 
> as well as amending the commit message to mention the manual touchup
> needed to work around the (in our opinion, misguided) advice from Go.
Thank you :)
Laszlo
> 
>>
>> ?
>>
>> Asking because I suspect that gofmt's purpose with the whitespace
>> removal around "+" is to emphasize that "+" binds
more strongly than
>> "!=". And, if we forced the binding with () around +, then
gofmt might
>> not feel the need to emphasize the difference between the binding
strengths.
>>
>> Acked-by: Laszlo Ersek <lersek at redhat.com>
>>
>> Laszlo
>>
>

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 at redhat.com>
> > > ---
> > > +++ 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.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|