|
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. 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. Regards 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. See: https://gist.github.com/briandfoy/4525877 https://www.securecoding.cert.org/confluence/display/perl/SEI+CERT+Perl+Coding+Standard https://matrix.to/#/!qyoLumPqusaXqFJNyK:matrix.org/$14917685211662024izFqM:matrix.org https://matrix.to/#/!qyoLumPqusaXqFJNyK:matrix.org/$1491849623220407yWMci:matrix.org 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' => 172, 'Perl::Critic::Policy::Modules::RequireVersionVar' => 170, 'Perl::Critic::Policy::ErrorHandling::RequireCarping' => 122, 'Perl::Critic::Policy::Subroutines::ProhibitUnusedPrivateSubroutines' => 34, 'Perl::Critic::Policy::InputOutput::RequireCheckedSyscalls' => 31, 'Perl::Critic::Policy::Variables::RequireInitializationForLocalVars' => 26, 'Perl::Critic::Policy::Subroutines::ProhibitBuiltinHomonyms' => 24 'Perl::Critic::Policy::ValuesAndExpressions::ProhibitMixedBooleanOperators' => 23, 'Perl::Critic::Policy::Subroutines::ProhibitExplicitReturnUndef' => 21, 'Perl::Critic::Policy::RegularExpressions::ProhibitCaptureWithoutTest' => 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' => 10, 'Perl::Critic::Policy::InputOutput::ProhibitBarewordFileHandles' => 10, 'Perl::Critic::Policy::ValuesAndExpressions::ProhibitMismatchedOperators' => 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, 'Perl::Critic::Policy::ValuesAndExpressions::ProhibitCommaSeparatedStatements' => 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: https://ledgersmb.org/community-guide/community-guide/development/coding-guidelines/perl-coding-guidelines 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! Nick |
------------------------------------------------------------------------------ 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 ..hidden.. https://lists.sourceforge.net/lists/listinfo/ledger-smb-devel