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

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.