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

On Wed, Jul 26, 2023 at 12:50:03PM -0500, 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.
Touching up manually isn't a good idea if you want to enforce
'go fmt' compliance, as you'll want contributors to be able to
set their editors to automatically run 'go fmt' upon save and
submit as is.
> 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.
libvirt-ci provides a container image for running & reporting
go fmt violations. Since you're using lcitool manifest, just
make this change

$ git diff ci/manifest.yml
diff --git a/ci/manifest.yml b/ci/manifest.yml
index f7b1daf..a5f3e66 100644
--- a/ci/manifest.yml
+++ b/ci/manifest.yml
@@ -6,6 +6,7 @@ gitlab:
   project: libnbd
   jobs:
     check-dco: false
+    go-fmt: true
 
 targets:
   alpine-315: x86_64


and re-generate, and it'll enforce style on *.go tree-wide.

libvirt-ci manifest also supports 'cargo-fmt' for rust, 'black'
for
python, and 'clang-format' for C too, enabled in the same way.


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

On Thu, Jul 27, 2023 at 04:00:25PM +0100, Daniel P. Berrang?
wrote:
> On Wed, Jul 26, 2023 at 12:50:03PM -0500, 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.
> 
> Touching up manually isn't a good idea if you want to enforce
> 'go fmt' compliance, as you'll want contributors to be able to
> set their editors to automatically run 'go fmt' upon save and
> submit as is.
Maybe I need to improve my wording here.  The existing format was
ambiguous enough that gofmt picked a different layout than our
original intent; manually tweaking was done to first get the comments
in a format that better matched our original intent, at which point
gofmt no longer mangled the comment.  Either way, the end result is
still something that gofmt approves of.  And yes, the goal is that in
the future, we will avoid commits where we add code to .go files that
gofmt does not like.
> 
> > 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.
> 
> libvirt-ci provides a container image for running & reporting
> go fmt violations. Since you're using lcitool manifest, just
> make this change
> 
> $ git diff ci/manifest.yml
> diff --git a/ci/manifest.yml b/ci/manifest.yml
> index f7b1daf..a5f3e66 100644
> --- a/ci/manifest.yml
> +++ b/ci/manifest.yml
> @@ -6,6 +6,7 @@ gitlab:
>    project: libnbd
>    jobs:
>      check-dco: false
> +    go-fmt: true
>  
>  targets:
>    alpine-315: x86_64
> 
> 
> and re-generate, and it'll enforce style on *.go tree-wide.
> 
> libvirt-ci manifest also supports 'cargo-fmt' for rust,
'black' for
> python, and 'clang-format' for C too, enabled in the same way.
Nice. That is shorter than my patch 7/7 approach.  It does two slight
drawbacks, though:

- It only covers checked-in .go files, whereas my patch can also
enforce formatting of generated .go files, if we want the generator to
be complex enough to guarantee well-formatted output

- failures are not detected at local 'make check' time, but rather
delayed until a CI run. That's fine if your development model is pull
request first; but a bit painful if it is commit email patches first
and then see how CI fares.

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