Add --check option to pgindent

On 12 Dec 2023, at 01:09, Tristan Partin <tristan@neon.tech> wrote:

Not sold on the name, but --check is a combination of --silent-diff and --show-diff. I envision --check mostly being used in CI environments. I recently came across a situation where this behavior would have been useful. Without --check, you're left to capture the output of --show-diff and exit 2 if the output isn't empty by yourself.

I haven’t studied the patch but I can see that being useful.

./daniel

On 12 Dec 2023, at 01:09, Tristan Partin <tristan@neon.tech> wrote:

Not sold on the name, but --check is a combination of --silent-diff and --show-diff. I envision --check mostly being used in CI environments. I recently came across a situation where this behavior would have been useful. Without --check, you're left to capture the output of --show-diff and exit 2 if the output isn't empty by yourself.

I wonder if we should model this around the semantics of git diff to keep it
similar to other CI jobs which often use git diff? git diff --check means "are
there conflicts or issues" which isn't really comparable to here, git diff
--exit-code however is pretty much exactly what this is trying to accomplish.

That would make pgindent --show-diff --exit-code exit with 1 if there were
diffs and 0 if there are no diffs.

--
Daniel Gustafsson

Hi,

On Tue, Dec 12, 2023 at 11:18:59AM +0100, Daniel Gustafsson wrote:

On 12 Dec 2023, at 01:09, Tristan Partin <tristan@neon.tech> wrote:

Not sold on the name, but --check is a combination of --silent-diff
and --show-diff. I envision --check mostly being used in CI
environments. I recently came across a situation where this behavior
would have been useful. Without --check, you're left to capture the
output of --show-diff and exit 2 if the output isn't empty by
yourself.

I wonder if we should model this around the semantics of git diff to
keep it similar to other CI jobs which often use git diff? git diff
--check means "are there conflicts or issues" which isn't really
comparable to here, git diff --exit-code however is pretty much
exactly what this is trying to accomplish.

That would make pgindent --show-diff --exit-code exit with 1 if there
were diffs and 0 if there are no diffs.

To be honest, I find that rather convoluted; contrary to "git diff", I
believe the primary action of pgident is not to show diffs, so I find
the proposed --check option to be entirely reasonable from a UX
perspective.

On the other hand, tying a "does this need re-indenting?" question to a
"--show-diff --exit-code" option combination is not very obvious (to me,
at least).

Cheers,

Michael

On Tue, Dec 12, 2023, at 7:28 AM, Michael Banck wrote:

On Tue, Dec 12, 2023 at 11:18:59AM +0100, Daniel Gustafsson wrote:

On 12 Dec 2023, at 01:09, Tristan Partin <tristan@neon.tech> wrote:

Not sold on the name, but --check is a combination of --silent-diff
and --show-diff. I envision --check mostly being used in CI
environments. I recently came across a situation where this behavior
would have been useful. Without --check, you're left to capture the
output of --show-diff and exit 2 if the output isn't empty by
yourself.

I wonder if we should model this around the semantics of git diff to
keep it similar to other CI jobs which often use git diff? git diff
--check means "are there conflicts or issues" which isn't really
comparable to here, git diff --exit-code however is pretty much
exactly what this is trying to accomplish.

That would make pgindent --show-diff --exit-code exit with 1 if there
were diffs and 0 if there are no diffs.

To be honest, I find that rather convoluted; contrary to "git diff", I
believe the primary action of pgident is not to show diffs, so I find
the proposed --check option to be entirely reasonable from a UX
perspective.

On the other hand, tying a "does this need re-indenting?" question to a
"--show-diff --exit-code" option combination is not very obvious (to me,
at least).

Multiple options to accomplish a use case might not be obvious. I'm wondering
if we can combine it into a unique option.

--show-diff             show the changes that would be made
--silent-diff           exit with status 2 if any changes would be made
+ --check                 combination of --show-diff and --silent-diff

I mean

--diff=show,silent,check

When you add exceptions, it starts to complicate the UI.

usage("Cannot have both --silent-diff and --show-diff")
if $silent_diff && $show_diff;

+usage("Cannot have both --check and --show-diff")
+  if $check && $show_diff;
+
+usage("Cannot have both --check and --silent-diff")
+  if $check && $silent_diff;
+

--
Euler Taveira
EDB https://www.enterprisedb.com/

"Euler Taveira" <euler@eulerto.com> writes:

When you add exceptions, it starts to complicate the UI.

Indeed. It seems like --silent-diff was poorly defined and poorly
named, and we need to rethink that option along the way to adding
this behavior. The idea that --show-diff and --silent-diff can
be used together is just inherently confusing, because they sound
like opposites.

regards, tom lane

On Tue Dec 12, 2023 at 5:47 AM CST, Euler Taveira wrote:

On Tue, Dec 12, 2023, at 7:28 AM, Michael Banck wrote:

On Tue, Dec 12, 2023 at 11:18:59AM +0100, Daniel Gustafsson wrote:

On 12 Dec 2023, at 01:09, Tristan Partin <tristan@neon.tech> wrote:

Not sold on the name, but --check is a combination of --silent-diff
and --show-diff. I envision --check mostly being used in CI
environments. I recently came across a situation where this behavior
would have been useful. Without --check, you're left to capture the
output of --show-diff and exit 2 if the output isn't empty by
yourself.

I wonder if we should model this around the semantics of git diff to
keep it similar to other CI jobs which often use git diff? git diff
--check means "are there conflicts or issues" which isn't really
comparable to here, git diff --exit-code however is pretty much
exactly what this is trying to accomplish.

That would make pgindent --show-diff --exit-code exit with 1 if there
were diffs and 0 if there are no diffs.

To be honest, I find that rather convoluted; contrary to "git diff", I
believe the primary action of pgident is not to show diffs, so I find
the proposed --check option to be entirely reasonable from a UX
perspective.

On the other hand, tying a "does this need re-indenting?" question to a
"--show-diff --exit-code" option combination is not very obvious (to me,
at least).

Multiple options to accomplish a use case might not be obvious. I'm wondering
if we can combine it into a unique option.

--show-diff             show the changes that would be made
--silent-diff           exit with status 2 if any changes would be made
+ --check                 combination of --show-diff and --silent-diff

I mean

--diff=show,silent,check

When you add exceptions, it starts to complicate the UI.

usage("Cannot have both --silent-diff and --show-diff")
if $silent_diff && $show_diff;

+usage("Cannot have both --check and --show-diff")
+  if $check && $show_diff;
+
+usage("Cannot have both --check and --silent-diff")
+  if $check && $silent_diff;
+

I definitely agree. I just didn't want to remove options, but if people
are ok with that, I will just change the interface.

I like the git-diff semantics or have --diff and --check, similar to how
Python's black[0]https://github.com/psf/black is.

[0]: https://github.com/psf/black

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

On 2023-Dec-12, Tom Lane wrote:

"Euler Taveira" <euler@eulerto.com> writes:

When you add exceptions, it starts to complicate the UI.

Indeed. It seems like --silent-diff was poorly defined and poorly
named, and we need to rethink that option along the way to adding
this behavior. The idea that --show-diff and --silent-diff can
be used together is just inherently confusing, because they sound
like opposites.

Maybe it's enough to rename --silent-diff to --check. You can do
"--show-diff --check" and get both the error and the diff printed; or
just "--check" and it'll throw an error without further ado; or
"--show-diff" and it will both apply the diff and print it.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"I must say, I am absolutely impressed with what pgsql's implementation of
VALUES allows me to do. It's kind of ridiculous how much "work" goes away in
my code. Too bad I can't do this at work (Oracle 8/9)." (Tom Allison)
http://archives.postgresql.org/pgsql-general/2007-06/msg00016.php

On 2023-12-12 Tu 10:30, Alvaro Herrera wrote:

On 2023-Dec-12, Tom Lane wrote:

"Euler Taveira" <euler@eulerto.com> writes:

When you add exceptions, it starts to complicate the UI.

Indeed. It seems like --silent-diff was poorly defined and poorly
named, and we need to rethink that option along the way to adding
this behavior. The idea that --show-diff and --silent-diff can
be used together is just inherently confusing, because they sound
like opposites

Maybe it's enough to rename --silent-diff to --check. You can do
"--show-diff --check" and get both the error and the diff printed; or
just "--check" and it'll throw an error without further ado; or
"--show-diff" and it will both apply the diff and print it.

That seems reasonable. These features were fairly substantially debated
when we put them in, but I'm fine with some tweaking. But note:
--show-diff doesn't apply the diff, it's intentionally non-destructive.

cheers

andrew

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

On Wed Dec 13, 2023 at 2:46 PM CST, Andrew Dunstan wrote:

On 2023-12-12 Tu 10:30, Alvaro Herrera wrote:

On 2023-Dec-12, Tom Lane wrote:

"Euler Taveira" <euler@eulerto.com> writes:

When you add exceptions, it starts to complicate the UI.

Indeed. It seems like --silent-diff was poorly defined and poorly
named, and we need to rethink that option along the way to adding
this behavior. The idea that --show-diff and --silent-diff can
be used together is just inherently confusing, because they sound
like opposites

Maybe it's enough to rename --silent-diff to --check. You can do
"--show-diff --check" and get both the error and the diff printed; or
just "--check" and it'll throw an error without further ado; or
"--show-diff" and it will both apply the diff and print it.

That seems reasonable. These features were fairly substantially debated
when we put them in, but I'm fine with some tweaking. But note:
--show-diff doesn't apply the diff, it's intentionally non-destructive.

Here is a new patch:

- Renames --silent-diff to --check
- Renames --show-diff to --diff
- Allows one to use --check and --diff in the same command

I am not tied to the second patch if people like --show-diff better than
--diff.

Weirdly enough, my email client doesn't show this as part of the
original thread, but I will respond here anyway.

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

This part of the first patch seems incorrect, i.e. same condition in
the if as elsif

-       if ($silent_diff)
+       if ($check)
+       {
+           print show_diff($source, $source_filename);
+           exit 2;
+       }
+       elsif ($check)
        {
            exit 2;
        }

On Fri Dec 15, 2023 at 8:18 AM CST, Jelte Fennema-Nio wrote:

This part of the first patch seems incorrect, i.e. same condition in
the if as elsif

-       if ($silent_diff)
+       if ($check)
+       {
+           print show_diff($source, $source_filename);
+           exit 2;
+       }
+       elsif ($check)
{
exit 2;
}

Weird how I was able to screw that up so bad :). Thanks! Here is a v3.
--
Tristan Partin
Neon (https://neon.tech)

On 15 Dec 2023, at 16:43, Tristan Partin <tristan@neon.tech> wrote:

Here is a v3.

I think this is pretty much ready to go, the attached v4 squashes the changes
and fixes the man-page which also needed an update. The referenced Wiki page
will need an edit or two after this goes in, but that's easy enough.

--
Daniel Gustafsson

On Mon, Dec 18, 2023, at 9:41 AM, Daniel Gustafsson wrote:

On 15 Dec 2023, at 16:43, Tristan Partin <tristan@neon.tech> wrote:

Here is a v3.

I think this is pretty much ready to go, the attached v4 squashes the changes
and fixes the man-page which also needed an update. The referenced Wiki page
will need an edit or two after this goes in, but that's easy enough.

... and pgbuildfarm client [1]https://github.com/PGBuildFarm/client-code/blob/main/PGBuild/Modules/CheckIndent.pm#L55-L90 needs to be changed too.

[1]: https://github.com/PGBuildFarm/client-code/blob/main/PGBuild/Modules/CheckIndent.pm#L55-L90

--
Euler Taveira
EDB https://www.enterprisedb.com/

On Mon Dec 18, 2023 at 6:41 AM CST, Daniel Gustafsson wrote:

On 15 Dec 2023, at 16:43, Tristan Partin <tristan@neon.tech> wrote:

Here is a v3.

I think this is pretty much ready to go, the attached v4 squashes the changes
and fixes the man-page which also needed an update. The referenced Wiki page
will need an edit or two after this goes in, but that's easy enough.

I have never edited the Wiki before. How can I do that? More than happy
to do it.

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

On Mon Dec 18, 2023 at 7:56 AM CST, Euler Taveira wrote:

On Mon, Dec 18, 2023, at 9:41 AM, Daniel Gustafsson wrote:

On 15 Dec 2023, at 16:43, Tristan Partin <tristan@neon.tech> wrote:

Here is a v3.

I think this is pretty much ready to go, the attached v4 squashes the changes
and fixes the man-page which also needed an update. The referenced Wiki page
will need an edit or two after this goes in, but that's easy enough.

... and pgbuildfarm client [1] needs to be changed too.

[1] https://github.com/PGBuildFarm/client-code/blob/main/PGBuild/Modules/CheckIndent.pm#L55-L90

Thanks Euler. I am on it.

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

On Mon, 18 Dec 2023 at 16:45, Tristan Partin <tristan@neon.tech> wrote:

On Mon Dec 18, 2023 at 6:41 AM CST, Daniel Gustafsson wrote:

On 15 Dec 2023, at 16:43, Tristan Partin <tristan@neon.tech> wrote:

Here is a v3.

I think this is pretty much ready to go, the attached v4 squashes the changes
and fixes the man-page which also needed an update. The referenced Wiki page
will need an edit or two after this goes in, but that's easy enough.

I have never edited the Wiki before. How can I do that? More than happy
to do it.

As mentioned at the bottom of the main page of the wiki:

NOTE: due to recent spam activity "editor" privileges are granted
manually for the time being.
If you just created a new community account or if your current
account used to have "editor" privileges ask on either the PostgreSQL
-www Mailinglist or the PostgreSQL IRC Channel (direct your request to
'pginfra', multiple individuals in the channel highlight on that
string) for help. Please include your community account name in those
requests.

After that, it's just a case of loggin in on the wiki (link top right
corner, which uses the community account) and then just go on to your
prefered page, click edit, and do your modifications. Don't forget to
save the modifications - I'm not sure whether the wiki editor's state
is locally persisted.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Mon, 18 Dec 2023 at 13:42, Daniel Gustafsson <daniel@yesql.se> wrote:

I think this is pretty much ready to go, the attached v4 squashes the changes
and fixes the man-page which also needed an update. The referenced Wiki page
will need an edit or two after this goes in, but that's easy enough.

One thing I'm wondering: When both --check and --diff are passed,
should pgindent still early exit with 2 on the first incorrectly
formatted file? Or should it show diffs for all failing files? I'm
leaning towards the latter making more sense.

Related (but not required for this patch): For my pre-commit hook I
would very much like it if there was an option to have pgindent write
the changes to disk, but still exit non-zero, e.g. a --write flag that
could be combined with --check just like --diff and --check can be
combined with this patch. Currently my pre-commit hook need two
separate calls to pgindent to both abort the commit and write the
required changes to disk.

On Mon, 18 Dec 2023 at 17:14, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:

One thing I'm wondering: When both --check and --diff are passed,
should pgindent still early exit with 2 on the first incorrectly
formatted file? Or should it show diffs for all failing files? I'm
leaning towards the latter making more sense.

To be clear, in the latter case it would still exit with 2 at the end
of the script, but only after showing diffs for all files.

On Mon Dec 18, 2023 at 10:14 AM CST, Jelte Fennema-Nio wrote:

On Mon, 18 Dec 2023 at 13:42, Daniel Gustafsson <daniel@yesql.se> wrote:

I think this is pretty much ready to go, the attached v4 squashes the changes
and fixes the man-page which also needed an update. The referenced Wiki page
will need an edit or two after this goes in, but that's easy enough.

One thing I'm wondering: When both --check and --diff are passed,
should pgindent still early exit with 2 on the first incorrectly
formatted file? Or should it show diffs for all failing files? I'm
leaning towards the latter making more sense.

Makes sense. Let me iterate on the squashed version of the patch.

Related (but not required for this patch): For my pre-commit hook I
would very much like it if there was an option to have pgindent write
the changes to disk, but still exit non-zero, e.g. a --write flag that
could be combined with --check just like --diff and --check can be
combined with this patch. Currently my pre-commit hook need two
separate calls to pgindent to both abort the commit and write the
required changes to disk.

I could propose something. It would help if I had an example of such
a tool that already exists.

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