Обсуждение: Add BF member koel-like indentation checks to SanityCheck CI

Поиск
Список
Период
Сортировка

Add BF member koel-like indentation checks to SanityCheck CI

От
Bharath Rupireddy
Дата:
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?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com



Add code indentation check to cirrus-ci (was Re: Add BF member koel-like indentation checks to SanityCheck CI)

От
Bharath Rupireddy
Дата:
On Mon, Oct 30, 2023 at 12:50 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?

Currently some of the code indentation issues that pgindent reports
are caught after the code gets committed either via the buildfarm
member koel or when someone runs pgindent before the code release.
These indentation issues are then fixed mostly by the committers in
separate commits. This is taking away the development time and causing
back and forth emails in mailing lists.

As of this writing, git log | grep 'koel' | wc -l gives 13 commits and
git log | grep 'indentation' | wc -l gives 100 commits (all may not be
related to code indentation per se). Almost 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.

The attached patch adds a new cirrus-ci task with minimal resources
(with 1 CPU and ccache 150MB) that fails when pgindent is not happy
with any of the changes. This helps catch code indentation issues in
development phase way before things get committed. This step can kick
in developers cirrus-ci runs in their own accounts if cirrus-ci is
enabled in their development repos. Otherwise, it can also be enabled
to kick in cfbot runs (I've not explored this yet).

If we don't want this new task to drain the free credits/run time that
cirrus-ci offers, one possible way is to cook the code indentation
check into either SanityCheck or CompilerWarnings task to save on the
resources. If at all an indentation check like this is needed, we can
think of adding pgperltidy check as well.

Here's a development branch to see the new task in action
https://github.com/BRupireddy2/postgres/tree/add_code_indentation_check_to_cirrus_ci
- an intentional pgindent failure is detected by the new task where
the diff is uploaded as an artifact -
https://api.cirrus-ci.com/v1/artifact/task/6127561344811008/indentation/pgindent.diffs.

Thoughts?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Вложения
Hi,

You may want to check out the WIP patch [1] about adding meson targets
to run pgindent by Andres.

[1] https://www.postgresql.org/message-id/20231019044907.ph6dw637loqg3lqk%40awork3.anarazel.de

-- 
Regards,
Nazir Bilal Yavuz
Microsoft



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

От
John Naylor
Дата:
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. 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.



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

От
Jelte Fennema-Nio
Дата:
From the previous thread on this issue. I think the following proposal
seemed like it had the most buy-in from committers. But so far nobody
implemented it:

On Wed, 18 Oct 2023 at 16:07, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Oct 18, 2023 at 3:21 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> > On 18.10.23 06:40, David Rowley wrote:
> > > I agree that it's not nice to add yet another way of breaking the
> > > buildfarm and even more so when the committer did make check-world
> > > before committing. We have --enable-tap-tests, we could have
> > > --enable-indent-checks and have pgindent check the code is correctly
> > > indented during make check-world. Then just not have
> > > --enable-indent-checks in CI.
> >
> > This approach seems like a good improvement, even independent of
> > everything else we might do about this.  Making it easier to use and
> > less likely to be forgotten.  Also, this way, non-committer contributors
> > can opt-in, if they want to earn bonus points.
>
> Yeah. I'm not going to push anything that doesn't pass make
> check-world, so this is appealing in that something that I'm already
> doing would (or could be configured to) catch this problem also.

Source:
https://www.postgresql.org/message-id/flat/CA%2BTgmobWXtSciC6hahE0J5w01D6Z3LPv9ctb5Ty_ory4m-NiXQ%40mail.gmail.com#c0534b1ad7ac4ef301cd431e8a222e6c

(CC Tristan since he was making changes to pgindent recently, and so I
had pinged him off-list on this exact topic before)



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

От
"Tristan Partin"
Дата:
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)



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

От
Robert Haas
Дата:
On Tue, Jan 9, 2024 at 2:20 PM Tristan Partin <tristan@neon.tech> wrote:
> > 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.

John might have his own answer to this, but here's mine: it's a pain
in the rear end. By the time I'm getting close to committing something
I try to ensure that everything I'm posting is indented. But for early
versions of work it adds a lot of paper-pushing with little
corresponding benefit. I've been doing this long enough that my
natural coding style is close to what pgindent would produce, but
figuring out how many tab stops are needed after a variable name to
make the result agree with pgindent's sentiments is not something I
can do reliably.

> 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.

I think we're basically committed to that at this point, and long have
been. Before koel started grumping, people would periodically pgindent
particular files because if you wanted to indent your new patch, you
had to run pgindent on the file and then back out the changes that
were due to the preexisting file contents rather than your patch. That
was maddening in its own way. The new system is annoying a slightly
different set of people for a slightly different set of reasons, but
everybody understands that in the end, it's all gonna get pgindented.

I also agree with you that the culture of Postgres development is hard
to change. This is the only OSS project that I've ever worked on, and
I still do it the same way I worked on code 30 years ago, except now I
use git instead of cvs. I can't imagine how we could modernize some of
our development practices without causing unacceptable collateral
damage, but maybe there's a way, and for sure the way we do things
around here is pretty far out of the 2023 mainstream. That's something
we should be grappling with, somehow.

--
Robert Haas
EDB: http://www.enterprisedb.com



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

От
"Tristan Partin"
Дата:
On Tue Jan 9, 2024 at 2:49 PM CST, Robert Haas wrote:
> On Tue, Jan 9, 2024 at 2:20 PM Tristan Partin <tristan@neon.tech> wrote:
> > > 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.
>
> John might have his own answer to this, but here's mine: it's a pain
> in the rear end. By the time I'm getting close to committing something
> I try to ensure that everything I'm posting is indented. But for early
> versions of work it adds a lot of paper-pushing with little
> corresponding benefit. I've been doing this long enough that my
> natural coding style is close to what pgindent would produce, but
> figuring out how many tab stops are needed after a variable name to
> make the result agree with pgindent's sentiments is not something I
> can do reliably.

Interesting that you think this way. I generally setup format on save in
my editors and never think about things again. I agree that the indents
after variables is the hardest thing to internalize!

> > 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.
>
> I think we're basically committed to that at this point, and long have
> been. Before koel started grumping, people would periodically pgindent
> particular files because if you wanted to indent your new patch, you
> had to run pgindent on the file and then back out the changes that
> were due to the preexisting file contents rather than your patch. That
> was maddening in its own way. The new system is annoying a slightly
> different set of people for a slightly different set of reasons, but
> everybody understands that in the end, it's all gonna get pgindented.

I've seen this in the git-blame-ignore-revs file. Good to know the
historical context.

> I also agree with you that the culture of Postgres development is hard
> to change. This is the only OSS project that I've ever worked on, and
> I still do it the same way I worked on code 30 years ago, except now I
> use git instead of cvs. I can't imagine how we could modernize some of
> our development practices without causing unacceptable collateral
> damage, but maybe there's a way, and for sure the way we do things
> around here is pretty far out of the 2023 mainstream. That's something
> we should be grappling with, somehow.

I'm just a newcomer, but I have had some ideas that _don't_ involve
leaving the mailing list paradigm behind, but I will leave those for
another day and another thread :). Perhaps it is worth a talk at
a conference sometime.

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



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

От
Tom Lane
Дата:
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jan 9, 2024 at 2:20 PM Tristan Partin <tristan@neon.tech> wrote:
>>> 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.

> John might have his own answer to this, but here's mine: it's a pain
> in the rear end. By the time I'm getting close to committing something
> I try to ensure that everything I'm posting is indented. But for early
> versions of work it adds a lot of paper-pushing with little
> corresponding benefit. I've been doing this long enough that my
> natural coding style is close to what pgindent would produce, but
> figuring out how many tab stops are needed after a variable name to
> make the result agree with pgindent's sentiments is not something I
> can do reliably.

FWIW, I rely on Emacs C mode during initial development, and while
it's not far off from what pgindent does there are certain things it
doesn't match (notably, alignment of variable declarations).  I just
don't worry about that at that stage.  Once I have something that's
turning over, I'll pgindent it before final review and showing it to
other people.  That's mostly because I've been reading Postgres code
for so long that anything that isn't pgindented looks subtly wrong,
so reviewing it annoys my hindbrain.  But trying to match pgindent's
rules by hand, in an editor that doesn't provide help for that, is not
worth the mental effort.

>> 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.

> I think we're basically committed to that at this point, and long have
> been.

Agreed.  What's at stake here is not whether the final product will
be pgindented, but when that happens and who's responsible for making
it happen.  We're trying to switch from "fix it once a year or so"
to "make sure it's right at the point of commit", which is a problem
for committers who up to now weren't in the habit of automatically
pgindenting.  I don't think it's time to give up on the project of
changing those habits; and if we do give up, the answer surely must
not be to push the problem further upstream.  Occasional contributors
are even less likely to be able to cope with this.

In short, I don't think that putting this into CI is the answer.
Putting it into committers' standard workflow is a better idea,
if we can get all the committers on board with that.

            regards, tom lane



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

От
Daniel Gustafsson
Дата:
> On 9 Jan 2024, at 22:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> In short, I don't think that putting this into CI is the answer.
> Putting it into committers' standard workflow is a better idea,
> if we can get all the committers on board with that.

+many

--
Daniel Gustafsson




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

От
Jim Nasby
Дата:
On 1/9/24 3:20 PM, Tom Lane wrote:
> In short, I don't think that putting this into CI is the answer.
> Putting it into committers' standard workflow is a better idea,
> if we can get all the committers on board with that.

FWIW, that's the approach that go takes - not only for committing to go 
itself, but it is *strongly* recommended[1] that anyone writing any code 
in go makes running `go fmt` a standard part of their workflow. In my 
experience, it makes collaborating noticably easier because you never 
need to worry about formatting differences. FYI, vim makes this easy via 
vim-autoformat[2] (which also supports line-by-line formatting if the 
format tool allows it); presumably any modern editor has similar support.

1: Literally 3rd item at https://go.dev/doc/effective_go
2: https://github.com/vim-autoformat/vim-autoformat
-- 
Jim Nasby, Data Architect, Austin TX




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

От
Robert Haas
Дата:
On Tue, Jan 9, 2024 at 4:42 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> > On 9 Jan 2024, at 22:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > In short, I don't think that putting this into CI is the answer.
> > Putting it into committers' standard workflow is a better idea,
> > if we can get all the committers on board with that.
>
> +many

I think we need to do that, too, but the question is how. The best
suggestion I've heard so far was to make it part of the build, or part
of the test suite, so that if you don't do it, some part of what you
were going to do anyway actually fails. That avoids making it an extra
step that you have to remember separately. We have an absolutely
insane number of things-you-must-always-remember-to-do.

--
Robert Haas
EDB: http://www.enterprisedb.com



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

От
Tom Lane
Дата:
Robert Haas <robertmhaas@gmail.com> writes:
> I think we need to do that, too, but the question is how. The best
> suggestion I've heard so far was to make it part of the build, or part
> of the test suite, so that if you don't do it, some part of what you
> were going to do anyway actually fails. That avoids making it an extra
> step that you have to remember separately. We have an absolutely
> insane number of things-you-must-always-remember-to-do.

I thought we had a consensus that there should be a way to enable
running it as part of check-world, or some related target.  But
nobody's written that patch yet.

            regards, tom lane



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

От
John Naylor
Дата:
On Wed, Jan 10, 2024 at 2:20 AM Tristan Partin <tristan@neon.tech> wrote:
>
> On Tue Jan 9, 2024 at 3:00 AM CST, John Naylor wrote:
> > 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.

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.



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

От
Tom Lane
Дата:
John Naylor <johncnaylorls@gmail.com> writes:
> 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.

+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



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

От
Michael Paquier
Дата:
On Wed, Jan 10, 2024 at 01:25:36AM -0500, Tom Lane wrote:
> John Naylor <johncnaylorls@gmail.com> writes:
>> 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.
>
> +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...)

I don't do the debug stuff on the left margin even if I force my way
with custom elogs when running regression tests as it is quicker than
using a coverage report.  I also take notes while reviewing or
implementing things with the '//' comments, so seeing these gone after
a check run would be sad.

> 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.

Another possibility would be to hide the test behind a PG_TEST_EXTRA.
--
Michael

Вложения

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

От
Tom Lane
Дата:
Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jan 10, 2024 at 01:25:36AM -0500, Tom Lane wrote:
>> 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.

> Another possibility would be to hide the test behind a PG_TEST_EXTRA.

Yeah.  I'm not quite sure what's a good way to make this work, but
it seems like having "make check-world" always invoke it would not
be desirable.  Making that conditional on an environment variable
setting could be a better idea, perhaps?

            regards, tom lane



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

От
Bharath Rupireddy
Дата:
On Wed, Jan 10, 2024 at 12:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Michael Paquier <michael@paquier.xyz> writes:
> > On Wed, Jan 10, 2024 at 01:25:36AM -0500, Tom Lane wrote:
> >> 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.
>
> > Another possibility would be to hide the test behind a PG_TEST_EXTRA.
>
> Yeah.  I'm not quite sure what's a good way to make this work, but
> it seems like having "make check-world" always invoke it would not
> be desirable.  Making that conditional on an environment variable
> setting could be a better idea, perhaps?

It's easy to miss setting the environment variable and eventually end
up with code incompatible with pgindent committed. IMO, running the
pgindent in at least one of the CI systems if not all (either as part
task SyanityCheck or task Linux - Debian Bullseye - Autoconf) help
catches things early on in CF bot runs itself. This saves committers
time but at the cost of free run-time that cirrus-ci provides.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com



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

От
Tom Lane
Дата:
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> On Wed, Jan 10, 2024 at 12:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah.  I'm not quite sure what's a good way to make this work, but
>> it seems like having "make check-world" always invoke it would not
>> be desirable.  Making that conditional on an environment variable
>> setting could be a better idea, perhaps?

> It's easy to miss setting the environment variable and eventually end
> up with code incompatible with pgindent committed.

Well, we expect committers to know what they're doing.  I'm not
quite suggesting that committers add "export I_AM_A_PG_COMMITTER=1"
in their ~/.profile and then have the Makefiles check that to decide
what tests are invoked by "make check-world" ... but it doesn't
seem like a totally untenable idea, either.

> IMO, running the
> pgindent in at least one of the CI systems if not all (either as part
> task SyanityCheck or task Linux - Debian Bullseye - Autoconf) help
> catches things early on in CF bot runs itself. This saves committers
> time but at the cost of free run-time that cirrus-ci provides.

But that puts the burden of pgindent-cleanliness onto initial patch
submitters, which I think is the wrong thing for reasons mentioned
upthread.  We want to enforce this at commit into the master repo, but
I fear enforcing it earlier will drive novice contributors away for
no very good reason.

            regards, tom lane



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

От
"Tristan Partin"
Дата:
On Wed Jan 10, 2024 at 1:58 AM CST, Tom Lane wrote:
> Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> > IMO, running the
> > pgindent in at least one of the CI systems if not all (either as part
> > task SyanityCheck or task Linux - Debian Bullseye - Autoconf) help
> > catches things early on in CF bot runs itself. This saves committers
> > time but at the cost of free run-time that cirrus-ci provides.
>
> But that puts the burden of pgindent-cleanliness onto initial patch
> submitters, which I think is the wrong thing for reasons mentioned
> upthread.  We want to enforce this at commit into the master repo, but
> I fear enforcing it earlier will drive novice contributors away for
> no very good reason.

If we are worried about turning away novice contributors, there are much
bigger fish to fry than worrying if we will turn them away due to
requiring code to be formatted a certain way. Like I said earlier,
formatters are pretty common tools to be using these days. go fmt, deno
fmt, rustfmt, prettier, black, clang-format, uncrustify, etc.

Code formatting requirements are more likely to turn someone away from
contributing if code reviews are spent making numerous comments.
Luckily, we can just say "run this command line incantation of
pgindent," which in the grand scheme of things is easy compared to all
the other things you have to be aware of to contribute to Postgres.

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



2024-01 Commitfest.

Hi, This patch has a CF status of "Needs Review" [1], but it seems
like there was some CFbot test failure last time it was run [2].
Please have a look and post an updated version if necessary.

======
[1] https://commitfest.postgresql.org/46/4691/
[2] https://cirrus-ci.com/task/5033191522697216

Kind Regards,
Peter Smith.



On 2024-01-21 Su 22:19, Peter Smith wrote:
> 2024-01 Commitfest.
>
> Hi, This patch has a CF status of "Needs Review" [1], but it seems
> like there was some CFbot test failure last time it was run [2].
> Please have a look and post an updated version if necessary.
>

I don't think there's a consensus that we want this. It should probably 
be returned with feedback.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com




On Mon, Jan 22, 2024 at 8:48 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2024-01-21 Su 22:19, Peter Smith wrote:
> > 2024-01 Commitfest.
> >
> > Hi, This patch has a CF status of "Needs Review" [1], but it seems
> > like there was some CFbot test failure last time it was run [2].
> > Please have a look and post an updated version if necessary.
> >
>
> I don't think there's a consensus that we want this. It should probably
> be returned with feedback.

I've withdrawn the CF entry as the idea is being discussed in a
separate thread -
https://www.postgresql.org/message-id/20231019044907.ph6dw637loqg3lqk%40awork3.anarazel.de

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com