Обсуждение: Add --check option to pgindent
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. -- Tristan Partin Neon (https://neon.tech)
Вложения
> 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 usedin CI environments. I recently came across a situation where this behavior would have been useful. Without --check, you'releft 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 usedin CI environments. I recently came across a situation where this behavior would have been useful. Without --check, you'releft 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", Ibelieve the primary action of pgident is not to show diffs, so I findthe proposed --check option to be entirely reasonable from a UXperspective.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" <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] 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 Thu, 14 Dec 2023 at 17:54, Tristan Partin <tristan@neon.tech> wrote: > > 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)
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 changesand fixes the man-page which also needed an update. The referenced Wiki pagewill need an edit or two after this goes in, but that's easy enough.
... and pgbuildfarm client [1] needs to be changed too.
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)
On Mon, 18 Dec 2023 at 17:50, Tristan Partin <tristan@neon.tech> wrote: > I could propose something. It would help if I had an example of such > a tool that already exists. Basically the same behaviour as what you're trying to add now for --check, only instead of printing the diff it would actually change the files just like running pgindent without a --check flag does. i.e. allow pgindent --check to not run in "dry-run" mode My pre-commit hook looks like this currently (removed boring cruft around it): if ! src/tools/pgindent/pgindent --check $files > /dev/null; then exit 0 fi echo "Running pgindent on changed files" src/tools/pgindent/pgindent $files echo "Commit abandoned. Rerun git commit to adopt pgindent changes" exit 1 But I would like it to look like: if src/tools/pgindent/pgindent --check --write $files > /dev/null; then exit 0 fi echo "Commit abandoned. Rerun git commit to adopt pgindent changes" exit 1
On Mon Dec 18, 2023 at 10:50 AM CST, Tristan Partin wrote: > 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. Here is an additional patch which implements the behavior you described. The first patch is just Daniel's squashed version of my patches. -- Tristan Partin Neon (https://neon.tech)
Вложения
On Mon Dec 18, 2023 at 11:21 AM CST, Jelte Fennema-Nio wrote: > On Mon, 18 Dec 2023 at 17:50, Tristan Partin <tristan@neon.tech> wrote: > > I could propose something. It would help if I had an example of such > > a tool that already exists. > > Basically the same behaviour as what you're trying to add now for > --check, only instead of printing the diff it would actually change > the files just like running pgindent without a --check flag does. i.e. > allow pgindent --check to not run in "dry-run" mode > My pre-commit hook looks like this currently (removed boring cruft around it): > > if ! src/tools/pgindent/pgindent --check $files > /dev/null; then > exit 0 > fi > echo "Running pgindent on changed files" > src/tools/pgindent/pgindent $files > echo "Commit abandoned. Rerun git commit to adopt pgindent changes" > exit 1 > > But I would like it to look like: > > if src/tools/pgindent/pgindent --check --write $files > /dev/null; then > exit 0 > fi > echo "Commit abandoned. Rerun git commit to adopt pgindent changes" > exit 1 To me, the two options seem at odds, like you either check or write. How would you feel about just capturing the diffs that are printed and patch(1)-ing them? -- Tristan Partin Neon (https://neon.tech)
On 2023-12-18 Mo 11:14, 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. It should show them all. > > 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. Not sold on this. I don't want pgindent applied automatically to my uncommitted changes, but I do want a reminder that I forgot to run it. In any case, as you say it's a different topic. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
> On 19 Dec 2023, at 14:51, Andrew Dunstan <andrew@dunslane.net> wrote: > > On 2023-12-18 Mo 11:14, 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. > > It should show them all. Agreed. >> 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. > > Not sold on this. I don't want pgindent applied automatically to my uncommitted changes, but I do want a reminder thatI forgot to run it. In any case, as you say it's a different topic. I think we risk making the tool confusing if we add too many options which can all be combined to suit every need. The posted v5 seems like a good compromise I reckon. Andrew: When applying this, how do we synchronize with the buildfarm to avoid false negatives due to the BF using the wrong options? -- Daniel Gustafsson
On Mon Dec 18, 2023 at 3:18 PM CST, Tristan Partin wrote: > On Mon Dec 18, 2023 at 10:50 AM CST, Tristan Partin wrote: > > 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. > > Here is an additional patch which implements the behavior you described. > The first patch is just Daniel's squashed version of my patches. Assuming all this is good, I now have access to edit the Wiki. The PR for buildfarm client code is up, and hopefully that PR is correct. -- Tristan Partin Neon (https://neon.tech)
On Mon, 18 Dec 2023 at 22:18, Tristan Partin <tristan@neon.tech> wrote: > Here is an additional patch which implements the behavior you described. > The first patch is just Daniel's squashed version of my patches. I think we'd still want the early exit behaviour when only --check is provided. No need to spend time checking more files if you're not doing anything else. On Mon, 18 Dec 2023 at 22:34, Tristan Partin <tristan@neon.tech> wrote: > To me, the two options seem at odds, like you either check or write. How > would you feel about just capturing the diffs that are printed and > patch(1)-ing them? I tried capturing the diffs and patching them, but simply piping the pgindent output to patch(1) didn't work because the pipe loses the exit code of pgindent. -o pipefail would help with this, but it's not available in all shells. Also then it's suddenly unclear if pgident failed or if patch failed. Attached are two additional patches (in addition to your unchanged previous ones). One which adds the --write flag and one which early exits with --check when neither --write or --diff are provided. The additional code I added for the --write flag is really minimal IMO. On Tue, 19 Dec 2023 at 14:51, Andrew Dunstan <andrew@dunslane.net> wrote: > Not sold on this. I don't want pgindent applied automatically to my > uncommitted changes, but I do want a reminder that I forgot to run it. > In any case, as you say it's a different topic. To be clear, with this patch just passing --check won't apply the changes automatically. Only when passing both --write and --check will it write the files. To clarify my commit workflow is as follows: git add -p # interactively select all the changes that I want in my commit git commit # pre-commit hook fails because of pgindent and "fixes" my files in place but doesn't add them to the commit yet git add -p # I look at all the changes that pgindent make to see if they are sensible and accept them, if not I find some other way to fix them git commit # now commit succeeded On Tue, 19 Dec 2023 at 14:58, Daniel Gustafsson <daniel@yesql.se> wrote: > I think we risk making the tool confusing if we add too many options which can > all be combined to suit every need. The posted v5 seems like a good compromise > I reckon. I personally think these options are completely independent. So it's more confusing to me that they cannot be combined. I updated the help message in 0003 as well, to describe them as completely independent: --diff show the changes that need to be made --check exit with status 2 if any changes need to be made --write rewrites files that need changes (default if neither --check/--diff/--write are provided) PS. prettier (javascript formatter) allows both --check and --write at the same time to do exactly this PPS. the help message didn't contain anything about pgindent writing files by default (i.e. when --check or --diff are not provided) PPPS. I attached my new pre-commit hook for reference.
Вложения
On Tue Dec 19, 2023 at 10:36 AM CST, Jelte Fennema-Nio wrote: > On Mon, 18 Dec 2023 at 22:18, Tristan Partin <tristan@neon.tech> wrote: > > Here is an additional patch which implements the behavior you described. > > The first patch is just Daniel's squashed version of my patches. > > I think we'd still want the early exit behaviour when only --check is > provided. No need to spend time checking more files if you're not > doing anything else. Good point. Patch looks good. > On Mon, 18 Dec 2023 at 22:34, Tristan Partin <tristan@neon.tech> wrote: > > To me, the two options seem at odds, like you either check or write. How > > would you feel about just capturing the diffs that are printed and > > patch(1)-ing them? > > I tried capturing the diffs and patching them, but simply piping the > pgindent output to patch(1) didn't work because the pipe loses the > exit code of pgindent. -o pipefail would help with this, but it's not > available in all shells. Also then it's suddenly unclear if pgident > failed or if patch failed. I was envisioning something along the lines of: pgindent --check --diff > patches.txt status=$? patch <patches.txt # no idea if this works, or if you need a for loop with manual parsing exit $status -- Tristan Partin Neon (https://neon.tech)
On 2023-12-19 Tu 08:57, Daniel Gustafsson wrote: >> On 19 Dec 2023, at 14:51, Andrew Dunstan <andrew@dunslane.net> wrote: >> >> On 2023-12-18 Mo 11:14, 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. >> It should show them all. > Agreed. > >>> 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. >> Not sold on this. I don't want pgindent applied automatically to my uncommitted changes, but I do want a reminder thatI forgot to run it. In any case, as you say it's a different topic. > I think we risk making the tool confusing if we add too many options which can > all be combined to suit every need. The posted v5 seems like a good compromise > I reckon. > > Andrew: When applying this, how do we synchronize with the buildfarm to avoid > false negatives due to the BF using the wrong options? The only buildfarm animal involved here is koel, which I run, so the simplest way will be for me to commit the core patch and adjust the buildfarm code at the same time. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2023-12-20 We 11:05, Andrew Dunstan wrote: > > On 2023-12-19 Tu 08:57, Daniel Gustafsson wrote: >> The posted v5 seems like a good compromise >> I reckon. >> >> Andrew: When applying this, how do we synchronize with the buildfarm >> to avoid >> false negatives due to the BF using the wrong options? > > > The only buildfarm animal involved here is koel, which I run, so the > simplest way will be for me to commit the core patch and adjust the > buildfarm code at the same time. > > V5 seems to be the consensus. I went with that, but added in logic to exit the loop early for --check if we're not also doing --diff. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Tue, 19 Dec 2023 at 17:54, Tristan Partin <tristan@neon.tech> wrote: > I was envisioning something along the lines of: > > pgindent --check --diff > patches.txt > status=$? > patch <patches.txt # no idea if this works, or if you need a for loop with manual parsing > exit $status Okay, I got a working version. And I updated the pre-commit hook on the wiki accordingly.