Re: [Libguestfs] [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 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs