Re: cleaning perl code - Mailing list pgsql-hackers

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