Re: Add BF member koel-like indentation checks to SanityCheck CI

On Tue Jan 9, 2024 at 3:00 AM CST, John Naylor wrote:
> On Mon, Oct 30, 2023 at 2:21 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > Hi,
> >
> > How about adding code indent checks (like what BF member koel has) to
> > the SanityCheck CI task? This helps catch indentation problems way
> > before things are committed so that developers can find them out in
> > their respective CI runs and lets developers learn the postgres code
> > indentation stuff. It also saves committers time - git log | grep
> > 'koel' | wc -l gives me 11 commits and git log | grep 'indentation' |
> > wc -l gives me 97. Most, if not all of these commits went into fixing
> > code indentation problems that could have been reported a bit early
> > and fixed by developers/patch authors themselves.
> >
> > Thoughts?
>
> There are three possible avenues here:
>
> 1) Accept that there are going to be wrong indents committed sometimes
> 2) Write off buildfarm member koel as an experiment, remove it, and do
> reindents periodically. After having been "trained", committers will
> still make an effort to indent, and failure to do so won't be a
> house-on-fire situation.
> 3) The current proposal
>
> Number three is the least attractive option -- it makes everyone's
> development more demanding, with more CI failures where it's not
> helpful. If almost everything shows red in CI, that's too much noise,
> and a red sanity check will just start to be ignored.

You can't ignore something that has to be required. We could tell
committers that they shouldn't commit patches that don't pass pgindent,
which might even be the current case; I'm not sure.

> I don't indent during most of development, and don't intend to start.

Could you expand on why you don't? I could understand as you're writing,
but I would think formatting on save, might be useful.

> Inexperienced developers will think they have to jump through more
> hoops in order to get acceptance, making submissions more difficult,
> with no corresponding upside for them.

The modern developer is well accustomed to code formatting/linting
requirements. Languages that attract the average developer like Rust,
Python, and JavaScript all have well-known tools that many projects use
like rustfmt, black, or prettier.

> Also, imagine a CF manager sending 100 emails complaining about indentation.
> So, -1 from me.

Yes, this would be annoying for a CF manager, and for that reason,
I would agree with your assessment. But I think this issue speaks more
to how tooling around Postgres hacking works in general. For instance,
if we look at something like SourceHut, they send emails from their CI
to the patchset it tested, which gives submitters pretty immediate
feedback about whether their patch meets all the contributing
requirements. See the aerc-devel mailing list for an example[1].

I don't want to diminish the thankless work that goes into maintaining
the current tooling however. These aren't easy problems to solve, and
I know most people would rather hack on Postgres than cfbot, etc. Thanks
for keeping the Postgres lights on!

I think the current proposal is good if the development experience
around pgindent was better. I've tried to help with this. I created
a VSCode extension[0], which developers can use to auto-format Postgres
and extension source code if set up properly. My next plan is to
integrate pgindent into a Neovim workflow for myself, that I can maybe
package into a plugin for others. I'd also like to get to the suggestion
that Jelte sent about adding pgindent checks to check-world. In Meson,
I will add a run_target() for it too. If we can lower the burden of
running pgindent, the more chances that people will actually use it!

Projects of similarly large scope like LLVM manage to gate pull requests
on code formatting requirements, so it is definitely in the realm of
possibility. Unfortunately for Postgres, we are fighting an uphill
battle where life isn't as simple as opening a PR and GitHub Actions
tells you pretty quickly if your code isn't formatted properly. We don't
even run CI on all patches that get submitted to the list. They have to
be added to the commitfests. I know part of this is to save resources,
but maybe we could start manually running CI on patches on the list by
CCing cfbot or something. Just an idea.

Perhaps the hardest thing to change is the culture of Postgres
development. If we can't all agree that we want formatted code, then
there is no point in anything that I discussed.

[0]: https://marketplace.visualstudio.com/items?itemName=tristan957.postgres-hacker
[1]: https://lists.sr.ht/~rjarry/aerc-devel/patches/48415

--
Tristan Partin
Neon (https://neon.tech)