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

[Laszlo Ersek]

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
>>
>