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).
Regards,
--
-David
david@pgmasters.net