On Thu, Jul 27, 2023 at 11:07:07AM -0500, Eric Blake wrote: > 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
Yep, only committed files. > - 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. You can do both - in libvirt we have meson test check style, but in CI we have style checks split out into a separate job, as it makes it nicer when browsing CI failures to separate reporting of functional failures from style violations. 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 :| _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
