Re: perlcritic

> On 21 Mar 2017, at 19:20, David Steele <david(at)pgmasters(dot)net> wrote:
>
> On 3/6/17 12:02 PM, Dagfinn Ilmari Mannsåker wrote:
>> ilmari(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker) writes:
>>
>>> Hi Peter,
>>>
>>> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>>>
>>>> I posted this about 18 months ago but then ran out of steam. [ ] Here
>>>> is an updated patch. The testing instructions below still apply.
>>>> Especially welcome would be ideas on how to address some of the places
>>>> I have marked with ## no critic.
>>>
>>> Attached is a patch on top of yours that addresses all the ## no critic
>>> annotations except RequireFilenameMatchesPackage, which can't be fixed
>>> without more drastic reworking of the plperl build process.
>>>
>>> Tested on perl 5.8.1 and 5.24.0 by configuring with --with-perl and
>>> --enable-tap-tests followed by make check-world, and running pgindent
>>> --build.
>>
>> Attached is an updated version of the patch, in which
>> src/tools/msvc/gendef.pl actually compiles. If someone on Windows could
>> test it, that would be great.
>
> You are signed up to review this patch. Do you know when you'll have a chance to do that?

Below is a review of the two patches attached to the commitfest entry:

The v2-0001-Clean-up-Perl-code-according-to-perlcritic-severi.patch didn’t
apply cleanly due to later commits, but the fixes to get there were trivial.
The followup 0001-Fix-most-perlcritic-exceptions-v2.patch applied clean on top
of that. The attached patch contains these two patches, rebased on top of
current master, with the below small nitpicks.

Since the original submission, most things have been addressed already, leaving
this patch with mostly changing to three-close open. The no critic exceptions
left are quite harmless: two cases of RequireFilenameMatchesPackage and one
ProhibitStringyEval. All three could be fixed at the expense of complicating
things without much (or any) benefit (as mentioned up-thread by Dagfinn Ilmari
Mannsåker), so I’m fine with leaving them in.

A few small nitpicks on the patch:

## In src/interfaces/libpq/test/regress.pl:

-open(REGRESS_IN, "<", $regress_in)
+open(my $regress_in_fh, "<", $regress_in)

Reading and closing this file was still using REGRESS_IN, fixed in the attached
updated patch.

## In src/test/locale/sort-test.pl:

-open(INFILE, "<$ARGV[0]");
-chop(my (@words) = <INFILE>);
-close(INFILE);
+chop(my (@words) = <>);

While this hunk does provide the same functionality due to the magic handling
of ARGV in <>, it also carries the side “benefit” that arbitrary applications
can be executed by using a | to read the output from a program:

$ src/test/locale/sort-test.pl "rm README |"

$ cat README
cat: README: No such file or directory

A silly example for sure, but since the intent of the patch is to apply best
practices and safe practices, I’d argue that a normal three-clause open is
safer here. The risk for misuse is very low, but it also makes the code less
magic and more readable IMO.

Reading the thread, most of the discussion was around the use of three-clause
open instead of the older two-clause. Without diving into the arguments, there
are a few places where we should use three-clause open, so simply applying it
everywhere rather than on a case by case basis seems reasonable to me.
Consistency across the codebase helps when reading the code.

There is no measurable performance impact on the changes, and no user visible
changes to functionality. With this applied, make check-world passes and
perlcritic returns a clean run (except on the autogenerated Gen_dummy_probes.pl
which is kept out of this work). The intent of the patch is to make the code
consistent and readable, and it achieves that. I see no reason to not go ahead
with these changes, if only to keep the codebase consistent with with modern
Perl code is expected to look like.

Given the nitpick nature of the comments, bumping status to ready for
committer.

cheers ./daniel