[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Perl::Critic Tests

Hi Nick,

I think patches would be very desirable at least for master (with the possibility of backporting for 1.5 if they are not too invasive).

I would prefer to see them as a single Critic Policy per patch, with perhaps no more than 50 items addressed per patch.
That should limit the impact and make review easier

It should also help with back porting if we choose to do so as many of the patches should apply cleanly

Personally, I'd start with the Policies with the highest count, and work down through the list from there.
That should allow maximum results for minimum effort. As well as a faster reduction of violations.
eg: Adding a "final return" to subroutines should be (in most cases) as simple as adding a return statement at the end of every effected sub()

Keep in mind that depending on the "running" perl version we may need to add specific policy violation exceptions.
Running the critic tests on perl 5.22 may throw a violation that we can't resolve if we wish to retain 5.10 compatibility.

I'd suggest that we probably need to do a comparison between the critic results when running on both the earliest perl version we support (5.10 from memory) and the latest available perl version (5.24) to see how much impact that's likely to have.

As for the RequireVersionVar policy, I agree that we should have version numbers in ALL of our modules, but exactly how to handle this gets somewhat complicated.
Some info on this can be found at....

That last link states
$VERSION should be defined in the form....
    our $VERSION = 1.0611;
    $MyPackage::VERSION = 1.061;
    use vars qw($VERSION);
    use version; our $VERSION = qv(1.0611);
but NOT as....
    my $VERSION = 1.0611;
Further it suggests a common practice of....
    our ($VERSION) = '$Revision$' =~ m{ \$Revision: \s+ (\S+) }x;
However a quick search didn't turn up any docs on "$Revision$"

I'd STRONGLY recommend a separate discussion about VERSION so we can work out a Project Level policy before making any changes to the files.

David G

On 15/04/17 02:02, Nick Prater wrote:
As discussed in chat, I've been following up on ehuelsmann's suggestion 
to test our code against the Perl::Critic policies recommended by CERT. 


Enabling the CERT recommended policies on our 'new' code results in 1050 
policy violations:

'Perl::Critic::Policy::Subroutines::RequireFinalReturn' => 293,
'Perl::Critic::Policy::ValuesAndExpressions::ProhibitMagicNumbers' => 
'Perl::Critic::Policy::Modules::RequireVersionVar' => 170,
'Perl::Critic::Policy::ErrorHandling::RequireCarping' => 122,
'Perl::Critic::Policy::Subroutines::ProhibitUnusedPrivateSubroutines' => 
'Perl::Critic::Policy::InputOutput::RequireCheckedSyscalls' => 31,
'Perl::Critic::Policy::Variables::RequireInitializationForLocalVars' => 
'Perl::Critic::Policy::Subroutines::ProhibitBuiltinHomonyms' => 24
=> 23,
'Perl::Critic::Policy::Subroutines::ProhibitExplicitReturnUndef' => 21,
=> 14,
'Perl::Critic::Policy::InputOutput::RequireCheckedOpen' => 11,
'Perl::Critic::Policy::Objects::ProhibitIndirectSyntax' => 11,
'Perl::Critic::Policy::Variables::ProhibitUnusedVariables' => 10,
'Perl::Critic::Policy::TestingAndDebugging::ProhibitNoWarnings' => 10,
'Perl::Critic::Policy::Variables::RequireLocalizedPunctuationVars' => 
'Perl::Critic::Policy::InputOutput::ProhibitBarewordFileHandles' => 10,
=> 9,
'Perl::Critic::Policy::Subroutines::ProtectPrivateSubs' => 8,
'Perl::Critic::Policy::InputOutput::RequireCheckedClose' => 8,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitBooleanGrep' => 5,
'Perl::Critic::Policy::Variables::RequireLexicalLoopIterators' => 5,
=> 4,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitStringySplit' => 4,
'Perl::Critic::Policy::ControlStructures::ProhibitUnreachableCode' => 3,
'Perl::Critic::Policy::Variables::ProhibitConditionalDeclarations' => 3,
'Perl::Critic::Policy::TestingAndDebugging::ProhibitNoStrict' => 3,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitUniversalIsa' => 3,
'Perl::Critic::Policy::InputOutput::RequireEncodingWithUTF8Layer' => 2,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitStringyEval' => 1,

I've not tested 'old' code, as I view 'new' code as the first priority.

Our current coding guidelines are set out here:

Moving forward, Do we want to make these Perl::Critic policies part of 
our coding standard?

If yes, I'm happy to start preparing patches to fix these violations and 
add policies to the tests in xt/01.1-critic.t.

Happy Easter!


Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Ledger-smb-devel mailing list