Re: cleaning perl code - Mailing list pgsql-hackers

From Noah Misch
Subject Re: cleaning perl code
Date
Msg-id 20200416030101.GB977691@rfd.leadboat.com
Whole thread Raw
In response to Re: cleaning perl code  (Andrew Dunstan <andrew.dunstan@2ndquadrant.com>)
Responses Re: cleaning perl code
List pgsql-hackers
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.



pgsql-hackers by date:

Previous
From: David Rowley
Date:
Subject: Re: remove_useless_groupby_columns does not need to record constraint dependencies
Next
From: Kyotaro Horiguchi
Date:
Subject: Re: Allow pg_read_all_stats to read pg_stat_progress_*