Libguestfs - Jul 2023 - [libnbd PATCH 6/7] golang: Improve whitespace style in generated bindings

On 7/27/23 21:22, Eric Blake wrote:
> On Thu, Jul 27, 2023 at 08:50:31PM +0200, Laszlo Ersek wrote:
>>> File "GoLang.ml", line 186, characters 11-71:
>>> 186 |         pr ("\t" ^ "/* %s field is ignored
unless %sSet == true. */\n")
>>>                 
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> Error: This expression has type string but an expression was
expected of type
>>>          ('weak2 -> 'weak3 -> 'weak4, unit,
string, unit) format4 >>>            ('weak2 -> 'weak3 ->
'weak4, unit, string, string, string, unit)
>>>            CamlinternalFormatBasics.format6
>>
>> Sigh. :/
>>
>> "pr" is declared like this [generator/utils.mli]:
>>
>>   val pr : ('a, unit, string, unit) format4 -> 'a
>>
>> The format string would normally be converted to a "format4"
type, which
>> is a polymorphic type, taking one type variable ('a).
> 
> You dug even deeper than me.  Thanks, even if it didn't yield a
> trivial solution.
> 
>> Ultimately open-coding the \t escape sequences seems the least
intrusive...
>>
>>>
>>> so those are non-starters.  We can get more verbose with:
>>>
>>>     +        pr "\t";
>>>     +        pr "/* %s field is ignored unless %sSet == true.
*/\n"
>>>                fname fname;
>>>
>>> but that may not scale as nicely.  Go's use of TAB does not
specify
>>> what tabstop it prefers;
>>
>> well that I find unfathomable; how can any coding style mandate TABs
for
>> indentation if it doesn't explain how wide a TAB should be? For
example,
>> that makes "maximum line width" mostly impossible to define.
> 
> https://go.dev/doc/effective_go#formatting
> 
> |  Some formatting details remain. Very briefly:
> | 
> | Indentation
> |     We use tabs for indentation and gofmt emits them by default. Use
spaces only if you must.
> | Line length
> |     Go has no line length limit. Don't worry about overflowing a
punched card. If a line feels too long, wrap it and indent with an extra tab.
> 
> and no mention of tab width.  The Go community really expects you to
> use gofmt without questions.
I may be overly sensitive, but I don't appreciate the "punched
card"
reference. For work, I use a 22" monitor (landscape orientation), I like
to place two text windows (columns) side by side, and my eyesight isn't
the greatest, so I use relatively large fonts. My preferred source code
width is therefore 80 characters.

Other developers work with 100-120 chars per line; in edk2 we've seen
200+ chars even. When I complain, the answer is "just buy a larger
monitor, or use two monitors". Well, I don't feel *comfortable* using
two monitors (I've tried), *or* with a super wide screen (I've also got
a 24" monitor and that one feels too wide to me already -- but some
people use 49-50" monitors!).

It's strange that the go coding style tries to control the spacing
around operators like "+", but ignores the line length question (and
throws a quasi-insult at people that work with narrow source code). If a
codebase is consistently written with a 128 chars/line limit, it might
very well pass "gofmt", but I'd be mostly incapable of working
with it.
> 
>>
>>> our .editorconfig suggests using a tabstop of
>>> 4 (instead of the usual 8) when reading a .go file for less visual
>>> distraction, and which matches with GoLang.ml currently using
4-space
>>> indentation.
>>>
>>> Rich, do you have any ideas on any better approach to take here?
>>>
>>>>> -        pr "  %sSet bool\n" fname;
>>>>> -        pr "  %s " fname;
>>>>> +        pr "\t%sSet bool\n" fname;
>>>>> +        pr "\t%s    " fname;
>>>
>>> I also debated about splitting this patch into two (yes, more
>>> gruntwork): one to address space before '(', and the other
to address
>>> leading indentation.  Lines like this would be touched by both
>>> patches if I split.
>>>
>>
>> I wouldn't insist on such a split. :)
> 
> The benefit of such a split: changing "int (r)" to
"int(r)" is
> non-controversial, while changing "    line" to
"\tline" could be reverted
> if we find a cleaner way to get pr to do indentation on our behalf.
> 
> There's also the idea that if the main thing that gofmt still
> complains about is 4 spaces vs. TAB, we could use coreutils'
> unexpand(1) on platforms where 'gofmt' is not installed (although
I'm
> not sure unexpand is portable enough to consider it likely to be
> installed on other systems).  The more I think about this, the more
> I'm leaning towards injecting gofmt into the pipeline, especially
> since Tage has already proposed injecting rustfmt into the pipeline.
> 
> Still, cleaning up the ' (' to be consistent with language idioms
> seems worthwhile, even if I punt on the TAB issue.
> 
OK, sounds like a plan -- "separate out the space removal from before
parens, and integrate gofmt into the build process".

Regarding the new gofmt dependency: can we assume that wherever go
exists, gofmt also exists? (On RHEL9, they are both in golang-bin.) IOW,
whenever we build the go bindings, we can also format them. This seems
to have come up during the discussion, but I don't remember the verdict.

Laszlo

On Fri, Jul 28, 2023 at 10:29:42AM +0200, Laszlo Ersek
wrote:
> >>> I also debated about splitting this patch into two (yes, more
> >>> gruntwork): one to address space before '(', and the
other to address
> >>> leading indentation.  Lines like this would be touched by both
> >>> patches if I split.
> >>>
> >>
> >> I wouldn't insist on such a split. :)
> > 
> > The benefit of such a split: changing "int (r)" to
"int(r)" is
> > non-controversial, while changing "    line" to
"\tline" could be reverted
> > if we find a cleaner way to get pr to do indentation on our behalf.
> > 
> > There's also the idea that if the main thing that gofmt still
> > complains about is 4 spaces vs. TAB, we could use coreutils'
> > unexpand(1) on platforms where 'gofmt' is not installed
(although I'm
> > not sure unexpand is portable enough to consider it likely to be
> > installed on other systems).  The more I think about this, the more
> > I'm leaning towards injecting gofmt into the pipeline, especially
> > since Tage has already proposed injecting rustfmt into the pipeline.
> > 
> > Still, cleaning up the ' (' to be consistent with language
idioms
> > seems worthwhile, even if I punt on the TAB issue.
> > 
> 
> OK, sounds like a plan -- "separate out the space removal from before
> parens, and integrate gofmt into the build process".
I've pushed 1-3, 5-6 as 5c2fc3cc..bc68eae4 (splitting up a couple of
them, and without any \t in patch 6).  I'm still playing with Dan's
idea for using CI to run gofmt, and/or how to build gofmt into the
tool chain, rather than immediately pushing patch 7.  In the short
term, I'm moving back to my work on the 64-bit extension patches.
> 
> Regarding the new gofmt dependency: can we assume that wherever go
> exists, gofmt also exists? (On RHEL9, they are both in golang-bin.) IOW,
> whenever we build the go bindings, we can also format them. This seems
> to have come up during the discussion, but I don't remember the
verdict.
That has been my assumption as well, but a well-written build
dependency will tolerate gofmt being missing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

On Fri, Jul 28, 2023 at 10:29:42AM +0200, Laszlo Ersek
wrote:
> On 7/27/23 21:22, Eric Blake wrote:
> > On Thu, Jul 27, 2023 at 08:50:31PM +0200, Laszlo Ersek wrote:
> >>> File "GoLang.ml", line 186, characters 11-71:
> >>> 186 |         pr ("\t" ^ "/* %s field is
ignored unless %sSet == true. */\n")
> >>>                 
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>> Error: This expression has type string but an expression was
expected of type
> >>>          ('weak2 -> 'weak3 -> 'weak4, unit,
string, unit) format4 > >>>            ('weak2 -> 'weak3
-> 'weak4, unit, string, string, string, unit)
> >>>            CamlinternalFormatBasics.format6
> >>
> >> Sigh. :/
> >>
> >> "pr" is declared like this [generator/utils.mli]:
> >>
> >>   val pr : ('a, unit, string, unit) format4 -> 'a
> >>
> >> The format string would normally be converted to a
"format4" type, which
> >> is a polymorphic type, taking one type variable ('a).
> > 
> > You dug even deeper than me.  Thanks, even if it didn't yield a
> > trivial solution.
> > 
> >> Ultimately open-coding the \t escape sequences seems the least
intrusive...
> >>
> >>>
> >>> so those are non-starters.  We can get more verbose with:
> >>>
> >>>     +        pr "\t";
> >>>     +        pr "/* %s field is ignored unless %sSet ==
true. */\n"
> >>>                fname fname;
> >>>
> >>> but that may not scale as nicely.  Go's use of TAB does
not specify
> >>> what tabstop it prefers;
> >>
> >> well that I find unfathomable; how can any coding style mandate
TABs for
> >> indentation if it doesn't explain how wide a TAB should be?
For example,
> >> that makes "maximum line width" mostly impossible to
define.
> > 
> > https://go.dev/doc/effective_go#formatting
> > 
> > |  Some formatting details remain. Very briefly:
> > | 
> > | Indentation
> > |     We use tabs for indentation and gofmt emits them by default. Use
spaces only if you must.
> > | Line length
> > |     Go has no line length limit. Don't worry about overflowing a
punched card. If a line feels too long, wrap it and indent with an extra tab.
> > 
> > and no mention of tab width.  The Go community really expects you to
> > use gofmt without questions.
> 
> I may be overly sensitive, but I don't appreciate the "punched
card"
> reference. For work, I use a 22" monitor (landscape orientation), I
like
> to place two text windows (columns) side by side, and my eyesight isn't
> the greatest, so I use relatively large fonts. My preferred source code
> width is therefore 80 characters.
> 
> Other developers work with 100-120 chars per line; in edk2 we've seen
> 200+ chars even. When I complain, the answer is "just buy a larger
> monitor, or use two monitors". Well, I don't feel *comfortable*
using
> two monitors (I've tried), *or* with a super wide screen (I've also
got
> a 24" monitor and that one feels too wide to me already -- but some
> people use 49-50" monitors!).
Yes, there are good human factors why ~ 80 chars should be the maximum
width.  It's the reason why newspapers and magazines don't format
everything to the width of the whole page, as it would be absolutely
unreadable.  (You can however argue if 80 chars specifically is a good
value, rather than a historic accident.)

Rich.
> It's strange that the go coding style tries to control the spacing
> around operators like "+", but ignores the line length question
(and
> throws a quasi-insult at people that work with narrow source code). If a
> codebase is consistently written with a 128 chars/line limit, it might
> very well pass "gofmt", but I'd be mostly incapable of
working with it.
> 
> > 
> >>
> >>> our .editorconfig suggests using a tabstop of
> >>> 4 (instead of the usual 8) when reading a .go file for less
visual
> >>> distraction, and which matches with GoLang.ml currently using
4-space
> >>> indentation.
> >>>
> >>> Rich, do you have any ideas on any better approach to take
here?
> >>>
> >>>>> -        pr "  %sSet bool\n" fname;
> >>>>> -        pr "  %s " fname;
> >>>>> +        pr "\t%sSet bool\n" fname;
> >>>>> +        pr "\t%s    " fname;
> >>>
> >>> I also debated about splitting this patch into two (yes, more
> >>> gruntwork): one to address space before '(', and the
other to address
> >>> leading indentation.  Lines like this would be touched by both
> >>> patches if I split.
> >>>
> >>
> >> I wouldn't insist on such a split. :)
> > 
> > The benefit of such a split: changing "int (r)" to
"int(r)" is
> > non-controversial, while changing "    line" to
"\tline" could be reverted
> > if we find a cleaner way to get pr to do indentation on our behalf.
> > 
> > There's also the idea that if the main thing that gofmt still
> > complains about is 4 spaces vs. TAB, we could use coreutils'
> > unexpand(1) on platforms where 'gofmt' is not installed
(although I'm
> > not sure unexpand is portable enough to consider it likely to be
> > installed on other systems).  The more I think about this, the more
> > I'm leaning towards injecting gofmt into the pipeline, especially
> > since Tage has already proposed injecting rustfmt into the pipeline.
> > 
> > Still, cleaning up the ' (' to be consistent with language
idioms
> > seems worthwhile, even if I punt on the TAB issue.
> > 
> 
> OK, sounds like a plan -- "separate out the space removal from before
> parens, and integrate gofmt into the build process".
> 
> Regarding the new gofmt dependency: can we assume that wherever go
> exists, gofmt also exists? (On RHEL9, they are both in golang-bin.) IOW,
> whenever we build the go bindings, we can also format them. This seems
> to have come up during the discussion, but I don't remember the
verdict.
> 
> Laszlo
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top