Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8; format=Flowed
Date: Tue, 09 Jan 2024 13:20:37 -0600
Message-Id: <CYAF3OCWIHZP.XA71OW47GOT5@neon.tech>
From: "Tristan Partin" <tristan@neon.tech>
Cc: "PostgreSQL Hackers" <pgsql-hackers@lists.postgresql.org>
Subject: Re: Add BF member koel-like indentation checks to SanityCheck CI
To: "John Naylor" <johncnaylorls@gmail.com>, "Bharath Rupireddy"
 <bharath.rupireddyforpostgres@gmail.com>, "Jelte Fennema-Nio"
 <postgres@jeltef.nl>
References: <CALj2ACWMupLTuD+bWfr2oBsNDpFJiuFaCK17S3Ty0YJbywdSfA@mail.gmail.com> <CANWCAZZObZeN9VqgmMY+mk4F1kpkuj=JB65R+o_HHoFT8c+eKg@mail.gmail.com>
In-Reply-To: <CANWCAZZObZeN9VqgmMY+mk4F1kpkuj=JB65R+o_HHoFT8c+eKg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/CYAF3OCWIHZP.XA71OW47GOT5%40neon.tech>
Precedence: bulk

On Tue Jan 9, 2024 at 3:00 AM CST, John Naylor wrote:
> On Mon, Oct 30, 2023 at 2:21=E2=80=AFPM 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=20
committers that they shouldn't commit patches that don't pass pgindent,=20
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,=20
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=20
requirements. Languages that attract the average developer like Rust,=20
Python, and JavaScript all have well-known tools that many projects use=20
like rustfmt, black, or prettier.

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

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

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

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

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

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

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

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