pgindent vs. pgperltidy command-line arguments

On 25 May 2023, at 11:10, Peter Eisentraut <peter@eisentraut.org> wrote:

Also, pgperltidy accepts no arguments and always processes the whole tree. It would be nice if there were a way to process individual files or directories, like pgindent can.

+1, thanks! I've wanted that several times but never gotten around to doing
anything about it.

--
Daniel Gustafsson

Peter Eisentraut <peter@eisentraut.org> writes:

Until PG15, calling pgindent without arguments would process the whole
tree. Now you get
No files to process at ./src/tools/pgindent/pgindent line 372.
Is that intentional?

It was intentional, cf b16259b3c and the linked discussion.

Also, pgperltidy accepts no arguments and always processes the whole
tree. It would be nice if there were a way to process individual files
or directories, like pgindent can.

+1, although I wonder if we shouldn't follow pgindent's new lead
and require some argument(s).

Attached is a patch for this.
(It seems that it works ok to pass regular files (not directories) to
"find", but I'm not sure if it's portable.)

The POSIX spec for find(1) gives an example of applying find to
what they evidently intend to be a plain file:

if [ -n "$(find file1 -prune -newer file2)" ]; then
printf %s\\n "file1 is newer than file2"
fi

So while I don't see it written in so many words, I think you
can assume it's portable.

regards, tom lane

On 25.05.23 15:20, Tom Lane wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

Until PG15, calling pgindent without arguments would process the whole
tree. Now you get
No files to process at ./src/tools/pgindent/pgindent line 372.
Is that intentional?

It was intentional, cf b16259b3c and the linked discussion.

Also, pgperltidy accepts no arguments and always processes the whole
tree. It would be nice if there were a way to process individual files
or directories, like pgindent can.

+1, although I wonder if we shouldn't follow pgindent's new lead
and require some argument(s).

That makes sense to me. Here is a small update with this behavior
change and associated documentation update.

On 2023-06-14 We 03:37, Peter Eisentraut wrote:

On 25.05.23 15:20, Tom Lane wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

Until PG15, calling pgindent without arguments would process the whole
tree.  Now you get
No files to process at ./src/tools/pgindent/pgindent line 372.
Is that intentional?

It was intentional, cf b16259b3c and the linked discussion.

Also, pgperltidy accepts no arguments and always processes the whole
tree.  It would be nice if there were a way to process individual files
or directories, like pgindent can.

+1, although I wonder if we shouldn't follow pgindent's new lead
and require some argument(s).

That makes sense to me.  Here is a small update with this behavior
change and associated documentation update.

I'm intending to add some of the new pgindent features to pgperltidy.
Preparatory to that here's a rewrite of pgperltidy in perl - no new
features yet but it does remove the hardcoded path, and requires you to
pass in one or more files / directories as arguments.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

I'm intending to add some of the new pgindent features to
pgperltidy. Preparatory to that here's a rewrite of pgperltidy in perl -
no new features yet but it does remove the hardcoded path, and requires
you to pass in one or more files / directories as arguments.

Good idea, here's some comments.

#!/usr/bin/perl

# Copyright (c) 2023, PostgreSQL Global Development Group

# src/tools/pgindent/pgperltidy

use strict;
use warnings;

use File::Find;

my $perltidy = $ENV{PERLTIDY} || 'perltidy';

my @files;

die "No directories or files specified" unless @ARGV;

It's not really useful to have the file name and line in errors like
this, adding a "\n" to the end of the message suppresses that.

sub is_perl_exec
{
my $name = shift;
my $out = `file $name 2>/dev/null`;
return $out =~ /:.*perl[0-9]*\b/i;
}

my $wanted = sub {

my $name = $File::Find::name;
my ($dev, $ino, $mode, $nlink, $uid, $gid);

# check it's a plain file and either it has a perl extension (.p[lm])
# or it's executable and `file` thinks it's a perl script.

(($dev, $ino, $mode, $nlink, $uid, $gid) = lstat($_))
&& -f _
&& (/\.p[lm]$/ || ((($mode & 0100) == 0100) && is_perl_exec($_)))
&& push(@files, $name);
};

The core File::stat and Fcntl modules can make this neater:

use File::stat;
use Fcntl ':mode';

my $wanted = sub {
my $st;
push @files, $File::Find::name
if $st = lstat($_) && -f $st
&& (/\.p[lm]$/ || (($st->mode & S_IXUSR) && is_perl_exec($_)));
};

File::Find::find({ wanted => $wanted }, @ARGV);

my $list = join(" ", @files);

system "$perltidy --profile=src/tools/pgindent/perltidyrc $list";

It's better to use the list form of system, to avoid shell escaping
issues. Also, since this is the last thing in the script we might as
well exec it instead:

exec $perltidy, '--profile=src/tools/pgindent/perltidyrc', @files;

- ilmari

On 20.06.23 17:38, Andrew Dunstan wrote:

+1, although I wonder if we shouldn't follow pgindent's new lead
and require some argument(s).

That makes sense to me.  Here is a small update with this behavior
change and associated documentation update.

I'm intending to add some of the new pgindent features to pgperltidy.
Preparatory to that here's a rewrite of pgperltidy in perl - no new
features yet but it does remove the hardcoded path, and requires you to
pass in one or more files / directories as arguments.

Are you planning to touch pgperlcritic and pgperlsyncheck as well? If
not, part of my patch would still be useful. Maybe I should commit my
posted patch for PG16, to keep consistency with pgindent, and then your
work would presumably be considered for PG17.

On 2023-06-21 We 05:09, Peter Eisentraut wrote:

On 20.06.23 17:38, Andrew Dunstan wrote:

+1, although I wonder if we shouldn't follow pgindent's new lead
and require some argument(s).

That makes sense to me.  Here is a small update with this behavior
change and associated documentation update.

I'm intending to add some of the new pgindent features to pgperltidy.
Preparatory to that here's a rewrite of pgperltidy in perl - no new
features yet but it does remove the hardcoded path, and requires you
to pass in one or more files / directories as arguments.

Are you planning to touch pgperlcritic and pgperlsyncheck as well?

Yeah, it would make sense to.

If not, part of my patch would still be useful.  Maybe I should commit
my posted patch for PG16, to keep consistency with pgindent, and then
your work would presumably be considered for PG17.

That sounds like a good plan.

cheers

andrew

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

On 21.06.23 13:35, Andrew Dunstan wrote:

If not, part of my patch would still be useful.  Maybe I should commit
my posted patch for PG16, to keep consistency with pgindent, and then
your work would presumably be considered for PG17.

That sounds like a good plan.

done

On 2023-06-21 We 07:35, Andrew Dunstan wrote:

On 2023-06-21 We 05:09, Peter Eisentraut wrote:

On 20.06.23 17:38, Andrew Dunstan wrote:

+1, although I wonder if we shouldn't follow pgindent's new lead
and require some argument(s).

That makes sense to me.  Here is a small update with this behavior
change and associated documentation update.

I'm intending to add some of the new pgindent features to
pgperltidy. Preparatory to that here's a rewrite of pgperltidy in
perl - no new features yet but it does remove the hardcoded path,
and requires you to pass in one or more files / directories as
arguments.

Are you planning to touch pgperlcritic and pgperlsyncheck as well?

Yeah, it would make sense to.

Here's a patch that turns all these into perl scripts.

cheers

andrew

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