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

From: John Naylor <[email-masked]>
Date: Tue, 9 Jan 2024 16:00:45 +0700

On Mon, Oct 30, 2023 at 2:21 PM Bharath Rupireddy
<[email-masked]> 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. I don't indent
during most of development, and don't intend to start. 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. Also, imagine a CF manager sending 100
emails complaining about indentation.

So, -1 from me.

---

From: "Tristan Partin" <[email-masked]>
Date: Tue, 09 Jan 2024 13:20:37 -0600

On Tue Jan 9, 2024 at 3:00 AM CST, John Naylor wrote:
> 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)

---

From: John Naylor <[email-masked]>
Date: Wed, 10 Jan 2024 13:14:25 +0700

> but I would think formatting on save, might be useful.

Off the top of my head, I like to use '//' comments as quick notes to
myself that stand out from normal code comments, and I'm in the habit
of putting debug print statements flush against the left margin so
they're really obvious. Both of these would be wiped out by pgindent.

---

From: Tom Lane <[email-masked]>
Date: Wed, 10 Jan 2024 01:25:36 -0500

> they're really obvious. Both of these would be wiped out by pgindent.

+1.  I do both of those things, partly because pgindent would reformat
them so that it'd be obvious if I forgot to remove them.  (Yes, I
look at the diffs pgindent wants to make...)

So that leads to the conclusion that I wouldn't want an automatic
pgindent check to happen during "make all" or "make check", because
I want those things to succeed before I consider pgindent'ing.
Maybe it's okay to include it as part of check-world, but I'm
not quite sure about that either.

			regards, tom lane