Item1518: Perlcritic should give only a small set of known and acknowledged errors

Perlcritic (on trunk) should only give errors that are known and valid (for speed or some other reasons).

The current logfile can be found here: http://fosiki.com/Foswiki_trunk/Foswiki-PerlCritic.log

-- OlivierRaginel - 24 Apr 2009

For example:

• lib/Foswiki/Configure/Checker.pm: Code before strictures are enabled at line 5, column 1. See page 429 of PBP. (Severity: 5)
  • This means there is some code before use strict;, which is easily avoidable -- OlivierRaginel
• lib/Foswiki/Configure/Checker.pm: Private Member Data shouldn't be accessed directly. at line 189, column 9. Accessing an objects data directly breaks encapsulation and should be avoided. Example: $object->{ some_key }. (Severity: 5)
  • This is because one should use getter and setter. This requires a change of the entire API, and will probably be slower, so this should be left as-is -- OlivierRaginel
• lib/Foswiki/Configure/Checker.pm: Stricture disabled at line 200, column 13. See page 429 of PBP. (Severity: 5)
  • This means there was a no strict, probably to bypass some checking temporarily. This should be reviewed, but normally is OK -- OlivierRaginel
• lib/Foswiki/Configure/Checker.pm: Expression form of "eval" for something other than require at line 201, column 13. It's okay to use stringy eval to require a module by name, but otherwise it's probably a mistake.. (Severity: 5)
  • This means there was some eval "some code";. This is discouraged because it will only be compiled upon execution. So, unless it's some use Module, which shouldn't be compiled, otherwise it will fail, it should most likely be written: eval { some code } -- OlivierRaginel

-- OlivierRaginel - 25 Apr 2009

I've "fixed" these:

• Code before strictures are enabled. Page 429 of PBP
• "return" statement with explicit "undef". Page 199 of PBP
• Two-argument "open". Page 207 of PBP.

It revealed some interesting issues, like some typos, or some variables wrongly named, which I hope I fixed properly. For example:

diff --git a/core/lib/Foswiki/Search/Parser.pm b/core/lib/Foswiki/Search/Parser.
index e238f12..ddf18f8 100644
--- a/core/lib/Foswiki/Search/Parser.pm
+++ b/core/lib/Foswiki/Search/Parser.pm

@@ -107,7 +110,7 @@ sub parse {
           split( /[\s]+/, $searchString );                 # split on spaces
     }
 
-    $result = new Foswiki::Search::Node($search, \@tokens, $options );
+    my $result = new Foswiki::Search::Node($searchString, \@tokens, $options );
     return $result;
 }

Note the $search instead of $searchString.

Will try to continue working on this when I have some time to kill and no brain power available

-- OlivierRaginel - 20 May 2009

distro:7970b4b52b42 breaks TWiki::Attrs in TWikiCompatibilityPlugin with a

Can't use string ("Foswiki::Attrs::") as a symbol ref while "strict refs" in use at /home/www-data/foswiki/trunk/core/lib/TWiki/Attrs.pm line 9

-- MichaelDaum - 04 Jun 2009

Fixed by distro:a720e4adb750, and I've checked other modules from TCP, they all look OK.

Thanks for reporting that Micha.

-- OlivierRaginel - 05 Jun 2009

current status:

lib/

410 files. 2,692 subroutines/methods. 60,671 statements. 98,456 lines.

Average McCabe score of subroutines was 4.89.

6,646 violations. Violations per file was 16.210. Violations per statement was 0.110. Violations per line of code was 0.068.

6,646 severity 5 violations.

bin/

24 files. 34 subroutines/methods. 978 statements. 1,735 lines.

Average McCabe score of subroutines was 3.41.

33 violations. Violations per file was 1.375. Violations per statement was 0.034. Violations per line of code was 0.019.

33 severity 5 violations.

-- WillNorris - 29 Dec 2009

where does the Private Member Data shouldn't be accessed directly. at line 189, column 9.  Accessing an objects data directly breaks encapsulation and should be avoided.  Example: $object->{ some_key }.  (Severity: 5) error come from? i don't get these error messages in my log file when building locally (using autoBuildFoswiki.pl)

checking in --exclude=Variables::ProtectPrivateVars as my best guess...

(NOTE: https://rt.cpan.org/Ticket/Display.html?id=50727 says that the computed McCabe scores are too high)

-- WillNorris - 29 Dec 2009

HELP! log broken: http://fosiki.com/Foswiki_trunk/Foswiki-PerlCritic.log shows sh: /usr/local/bin/perlcritic: No such file or directory

-- WillNorris - 24 Mar 2010

ah, making a new build box meant it had moved to /usr/bin/perlcritic. should be fixed now

-- SvenDowideit - 30 Apr 2010

Closing this - checkins have been picked up in a release.

-- GeorgeClark - 09 Mar 2011