From: Noah Misch <noah@leadboat.com>
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: cleaning perl code
Date: Wed, 15 Apr 2020 20:01:01 -0700
Message-ID: <20200416030101.GB977691@rfd.leadboat.com> (raw)
In-Reply-To: <86b0e20d-efc1-4169-aeb1-be49b3a375ab@2ndQuadrant.com>
References: <20200414204432.GA22922@alvherre.pgsql>
<86b0e20d-efc1-4169-aeb1-be49b3a375ab@2ndQuadrant.com>
On Wed, Apr 15, 2020 at 03:43:36PM -0400, Andrew Dunstan wrote:
> On 4/14/20 4:44 PM, Alvaro Herrera wrote:
> > On 2020-Apr-14, Andrew Dunstan wrote:
> >> One of the things that's a bit sad is that perlcritic doesn't generally
> >> let you apply policies to a given set of files or files matching some
> >> pattern. It would be nice, for instance, to be able to apply some
> >> additional standards to strategic library files like PostgresNode.pm,
> >> TestLib.pm and Catalog.pm. There are good reasons as suggested upthread
> >> to apply higher standards to library files than to, say, a TAP test
> >> script. The only easy way I can see to do that would be to have two
> >> different perlcriticrc files and adjust pgperlcritic to make two runs.
> >> If people think that's worth it I'll put a little work into it. If not,
> >> I'll just leave things here.
> > I think being more strict about it in strategic files (I'd say that's
> > Catalog.pm plus src/test/perl/*.pm) might be a good idea. Maybe give it
> > a try and see what comes up.
>
> OK, in fact those files are in reasonably good shape. I also took a pass
> through the library files in src/tools/msvc, which had a few more issues.
It would be an unpleasant surprise to cause a perlcritic buildfarm failure by
moving a function, verbatim, from a non-strategic file to a strategic file.
Having two Perl style regimes in one tree is itself a liability.
> --- a/src/backend/catalog/Catalog.pm
> +++ b/src/backend/catalog/Catalog.pm
> @@ -67,7 +67,7 @@ sub ParseHeader
> if (!$is_client_code)
> {
> # Strip C-style comments.
> - s;/\*(.|\n)*\*/;;g;
> + s;/\*(?:.|\n)*\*/;;g;
This policy against unreferenced groups makes the code harder to read, and the
chance of preventing a bug is too low to justify that.
> --- a/src/tools/perlcheck/pgperlcritic
> +++ b/src/tools/perlcheck/pgperlcritic
> @@ -14,7 +14,21 @@ PERLCRITIC=${PERLCRITIC:-perlcritic}
>
> . src/tools/perlcheck/find_perl_files
>
> -find_perl_files | xargs $PERLCRITIC \
> +flist=`mktemp`
> +find_perl_files > $flist
> +
> +pattern='src/test/perl/|src/backend/catalog/Catalog.pm|src/tools/msvc/[^/]*.pm'
I don't find these files to be especially strategic, and I'm mostly shrugging
about the stricter policy's effect on code quality. -1 for this patch.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, andrew.dunstan@2ndquadrant.com, alvherre@2ndquadrant.com
Subject: Re: cleaning perl code
In-Reply-To: <20200416030101.GB977691@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox