From: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Subject: Re: cleaning perl code
To: David Steele <david@pgmasters.net>
Cc: Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
 Noah Misch <noah@leadboat.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <e56f6fb6-38d5-8450-577c-24eb5d63f2e1@2ndQuadrant.com>
 <20200411043014.GA590991@rfd.leadboat.com> <27481.1586618092@sss.pgh.pa.us>
 <CA+TgmobP4TQHvj3nLCYUnmeqBc6n3Xb-q+b3CfvmoVxWS0qydw@mail.gmail.com>
 <757f9c47-ad93-8ca0-49ea-2b2ced58d103@pgmasters.net>
Autocrypt: addr=andrew.dunstan@2ndQuadrant.com; prefer-encrypt=mutual;
 keydata=
 mQENBE7KWFkBCAClridxur2AIc7eW2AR7izbfp3EnNefie2HbLF0izW5Ik5UjX2HBXBx4syI
 gY6b0ugohXrr274+baoAlvSbq6cAoQuEVrk5IZFzt20b1Xkx65FwGSEj526yiKLocqkJceSq
 Xr9xcA5SGY+FZv441chh5SU92v4q6z+6LPpoHOh97ptAVXZYNTtU0LevyvD5lja0TzbvJm6C
 eFXitJfnm1pLEr0DGJCR/iUOl/N62Kh4855zZC7NHIjQHPOvV5Stz/l5ilDhvGVk+xkXFPys
 SjZoUr1rXhYLpiyi5sR0X9FHXT0KnGuz1F5ERO7ZTLSSQ6fJwPj6gOk9K+vvoKvoeql5ABEB
 AAG0L0FuZHJldyBEdW5zdGFuIDxhbmRyZXcuZHVuc3RhbkAybmRRdWFkcmFudC5jb20+iQFY
 BBMBAgBCAhsDBgsJCAcDAgYVCAIJCgsEFgIDAQIeAQIXgBYhBOQ+WEYd/Hy/RGkVpZn6f8tZ
 /DuBAhkBBQJd1pSYBQkSzqMsAAoJEJn6f8tZ/DuBJmEIAIVFBIC3qGqh+YTPVTkfowwxpHz0
 tZSPjZ6GS/5y62kgoZrZOVXw+Rg2ceuMM3a+In/RRjTx4bembgYqRKgzXlDfiNBJeq5ioVuN
 H1faDKE2lm5a6QkRQ2JU6DKf60fqSJGP5LisCUr1V0wz6EnlO31ArrKtXCNB+yk0KDAS6o6I
 Ql4pkXpr+vq4iilwJmiJOqTBam0LmQE7ulYCCYU8Z6MkpiSl+dO7QSramcGyxauQRI3a7gcq
 dpo8Vrv3gFTQDeMYxk7cqZp08pIVH5XR0+LgBz4X84f59rA/VAKlC/r6CyEQk59rvDLxCMyP
 2buoj51Z25N3MVghRDGcsU6mT7e5AQ0ETspYWQEIANGc4zQULOxhbqO2dyD51YhqCNRmm9oK
 Waqf+wmW4tpDe/VVcxAnNizd4LWCHfzpb5cHAtGkOPePMfzWVf6nvdF7d3eglbtf59+zG7O7
 llV0xSSoFiieQBsrGvqDInXYX/4mRRXMtyhM353/tixC9RWLs1oofyYmCPPXXY7h9R7en3B8
 BoVrRFcdzlIY/NFNhFGW/9dkEiGjgna2Rk6e15kln4ZvFBWUg23p93w/pqXcxY6+k/8TEk+C
 4R+M6w7o2PLGOjdZ+kPiUcw5H85zf/yZJwQXzisXaNduwWB6Vads9YC9dj6kPR1c4VGRqAaY
 L++LAEOqrlvm2TvqQqZRtnEAEQEAAYkBJQQYAQIADwIbDAUCXdaUtAUJEs6jVQAKCRCZ+n/L
 Wfw7gQi5B/9sHbmRloZNCWWwZGm+O9iG/JGQxS0WWa4GaZqzqSTWOft2/gO0Bgag89r1djq4
 PFgSZtzHvKQpqYz/7HkPrWh1jpcS83i/uYY1uyHXUUtpdEmn4buNObEtD0YNaJkGdhASsECq
 6akw+u7WJYmAHs/SpR6ok6HqLw7JHDVc2h5dViLfTk0qrgDa7sq/LVMoJje5Ct/Ku3YaWAV6
 YSmzoPP5BmPVFqP11OMQkCqurIu9vTLXKFWX623vCmqRDHJ6e5GCDHbH8USU+4wJBfPymZkd
 cA7NGm/mmWeJiWBFIDyjzYxrjku8ty9TZJVsaRXnS+OFQzXz9Eu2wTpKdHij46uY
Message-ID: <a7654dc2-ebe8-712e-c25d-146ef7d7b42f@2ndQuadrant.com>
Date: Sun, 12 Apr 2020 18:24:15 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <757f9c47-ad93-8ca0-49ea-2b2ced58d103@pgmasters.net>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 4/12/20 4:12 PM, David Steele wrote:
> On 4/12/20 3:22 PM, Robert Haas wrote:
>> On Sat, Apr 11, 2020 at 11:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Noah Misch <noah@leadboat.com> writes:
>>>> In summary, among those warnings, I see non-negative value in "Code
>>>> before
>>>> warnings are enabled" only.  While we're changing this, I propose
>>>> removing
>>>> Subroutines::RequireFinalReturn.
>>>
>>> If it's possible to turn off just that warning, then +several.
>>> It's routinely caused buildfarm failures, yet I can detect exactly
>>> no value in it.  If there were sufficient cross-procedural analysis
>>> backing it to detect whether any caller examines the subroutine's
>>> result value, then it'd be worth having.  But there isn't, so those
>>> extra returns are just pedantic verbosity.
>>
>> I agree with Noah's comment about CPAN: it would be worth being more
>> careful about things like this if we were writing code that was likely
>> to be used by a wide variety of people and a lot of code over which we
>> have no control and which we do not get to even see. But that's not
>> the case here. It does not seem worth stressing the authors of TAP
>> tests over such things.
>
> FWIW, pgBackRest used Perl Critic when we were distributing Perl code
> but stopped when our Perl code was only used for integration testing.
> Perhaps that was the wrong call but we decided the extra time required
> to run it was not worth the benefit. Most new test code is written in
> C and the Perl test code is primarily in maintenance mode now.
>
> When we did use Perl Critic we set it at level 1 (--brutal) and then
> wrote an exception file for the stuff we wanted to ignore. The
> advantage of this is that if new code violated a policy that did not
> already have an exception we could evaluate it and either add an
> exception or modify the code. In practice this was pretty rare, but we
> also had a short excuse for many exceptions and a list of exceptions
> that should be re-evaluated in the future.
>
> About the time we introduced Perl Critic we were already considering
> the C migration so most of the exceptions stayed.
>
> Just in case it is useful, I have attached our old policy file with
> exceptions and excuses (when we had one).
>
>

That's a pretty short list for --brutal, well done. I agree there is
value in keeping documented the policies you're not complying with.
Maybe the burden of that is too much for this use, that's up to the
project to decide.

For good or ill we now have a significant investment in perl code - I
just looked and it's 180 files with 38,135 LOC, and that's not counting
the catalog data files, so we have some interest in keeping it fairly clean.

I did something similar to what's above with the buildfarm code,
although on checking now I find it's a bit out of date for the sev 1 and
2 warnings, so I'm fixing that. Having said that, my normal target is
level 3.

The absolutely minimal things I want to do are a) fix the code that
we're agreed on fixing (use of warnings, idiomatic use of $/), and b)
fix the output format to include the name of the policy being violated.


cheers


andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services