Beefy Boxes and Bandwidth Generously Provided by pair Networks
Problems? Is your data what you think it is?

Request for Perl::Critic Testimonials

by jthalhammer (Friar)
on Jul 15, 2006 at 09:22 UTC ( #561416=perlmeditation: print w/replies, xml ) Need Help??

Esteemed Monks-

As some of you already know, Perl::Critic has been my passion, and next month Perl::Critic will be turning 1 year old! Perl::Critic got good coverage at YAPC::NA and in "The Perl Review", but I'm not really sure how it's doing in the wild, especially in commercial environments.

So if any of you use Perl::Critic at $work or on other projects, I'd love to hear about it. Why did you decide to use P::C? Has it made your code better or worse? How has it impacted your development process? Were there any obstacles along the way?

I might publish your comments on , or possibly even in one of the trade magazines. So if you'd rather not (or cannot) state where you work, then that's cool.


Replies are listed 'Best First'.
Re: Request for Perl::Critic Testimonials (some weirdnesses of the module)
by Ieronim (Friar) on Jul 15, 2006 at 18:34 UTC
    The most weird error messages i get from Perl::Critic are those:
    Regular expression without '/m' flag Regular expression without '/x' flag
    They look very funny when they both refer to the line like this:
    next if /^;/; #skip comments
    I think you can set a length limit for the regular expressions to receive a warning about /x flag. Maybe, regexes shorter than e.g. 7 characters do not need /x flag? Or even better—remove warnings for regexes containing less than 5 metacharacters, e.g. /(foo|bar)/? The regexes containing only ONE interpolated variable (like the one mentioned below) must not cause them, too.

    The warning about /m simply looks weird for me though I can understand why do you use it. But it must not appear to the lines like

    $string =~ /$re/;

    The next bug: the line
    local $" = '|';
    causes two warnings:
    Variable declared as 'local' Magic punctuation variable used

    while it must cause only the second warning! To use or not to use the punctuation variables is some kind of personal style, but not local'izing them is a severe error!

    A little tip about the web-interface: You could add a possibility for the user to re-sort your warnings list by severity or by categories. The list would also look much better if it was represented by a table rather than by a simple linklist :)

    Despite all my remarks you have done a great work ad i hope that my comments will help you to improve it :)

    Sorry for my bad English—it's not my native language :)

      next if /^;/; #skip comments
      It is complaining the xm switches are not included Change it to this,

      Update: Also wrong per Jeronim

      next if /^;/xms;
      local $" = '|';
      Don't use local variables, use my. Don't the special variables like $", use the English module.

      Updated: Suggested but not working code

      use English qw( -no_match_vars ); my $LIST_SEPERATOR = q{|};

      Wrong spelling and yes will break your Code, Both pointed out by Jeronim and Chromatic...

      use English qw( -no_match_vars ); { local $LIST_SEPARATOR = q{|}; }

      Would love to delete the post for just being wrong... Oh well :)

        This will get you through the critic...

        ... and break your code.

        use Test::More tests => 1; use English '-no_match_vars'; my $LIST_SEPARATOR = '|'; my @items = qw( foo bar baz ); my $interpolated = "@items"; is( $interpolated, 'foo|bar|baz', 'somehow package globals and lexical +s merged' );
        You decided that my English is so poor that i can't read the docs of Perl::Critic and understand why these warnings appeared? ;)

        Your comment contains many errors :( Yes, if i modify my code according to your recommendations, Perl::Critic will stop complaining—but my code will stop working :)

        Your recommendation to use

        next if /^;/xms;
        is very weird. The using of /s and /m switches together causes a very unexpected interpretation of the RegEx:

        perlretut says:
        both s and m modifiers (//sm): Treat string as a single long line, but detect multiple lines. '.' matches any character, even "\n". ^ and $, however, are able to match at the start or end of any line within the string.

        Do you still think that it's a good idea to add /ms to the end of each regex?

        Don't use local variables, use my. Don't the special variables like $", use the English module.
        use English qw( -no_match_vars ); my $LIST_SEPERATOR = q{|};

        1. The Perl built-in variables lose their magic capabilities when declared as my. Didn't you know?
        2. There is no $LIST_SEPERATOR. Use $LIST_SEPARATOR instead.
        3. If you don't believe me, try the following code:
          #!/usr/bin/perl use warnings; use strict; use English qw( -no_match_vars ); { local $LIST_SEPARATOR = q{!}; my @list = qw/this is my list/; print "@list\n"; }
          and then try to replace 'local' with 'my'.
          BTW, Perl::Critic does not complain about local'ising in the code above ;)

              s;;Just-me-not-h-Ni-m-P-Ni-lm-I-ar-O-Ni;;tr?IerONim-?HAcker ?d;print
Re: Request for Perl::Critic Testimonials
by Herkum (Parson) on Jul 15, 2006 at 12:11 UTC


    Howdy, I have been using your module for all my code. Currently I turn off two things for Perl::Critic,

    1. RCS Control: am not using Subversion or CVS so I don't need that information in my modules.
    2. Tidy: I like tidy, however, I only recently got an email that PerlTidy had fixed a bug with its handling of attributes (it was breaking my Class::Std code).

    Otherwise I will accept 95% of the suggestions submitted by Perl::Critic and tell it to ignore the other 5% (generally this has to do with concating single and double quoted strings.

    Some people complain about the rules used by Perl::Critic, most of the complaints are about personal preference rather than the rule itself. I am willing to give up some of my preferences, which I am not particularly passionate about, to have very consistent code. I have found it easier to read and maintain code that I had write months ago when I did this.

    Overall, I have been very happy with the code I have written and validated by Perl::Critic and will continue to use it in the future.

Re: Request for Perl::Critic Testimonials
by perrin (Chancellor) on Jul 15, 2006 at 17:06 UTC
    I'm not using it yet, but I wanted to say that your web interface for it is pretty cool, and handy. Thanks for putting that up. If you add checkboxes for choosing the tests, it will be even cooler.
Re: Request for Perl::Critic Testimonials
by Crackers2 (Parson) on Jul 16, 2006 at 00:51 UTC

    The following:

     my $deg2rad = .017453293;

    gets me this (i believe incorrect) warning:
    Integer with leading zeros at line 33, column 16. See page 55 of PBP. Severity: 5

    Even just declaring @EXPORT results in Symbols are exported by default even when none are actually exported. Just rewording the warning for this case is probably enough.

    When using @ISA, I get a warning both on the declaration and on usage. For @EXPORT I only get a warning on the declaration. Harmless but inconsistent.

    Other than that, I think it appears to be pretty useful judging by the couple of checks I ran through the website.

Re: Request for Perl::Critic Testimonials
by Herkum (Parson) on Jul 16, 2006 at 23:10 UTC

    I also wanted to say this about Jeff. Damian Conway wrote the book (Perl Best Practices) but Jeff implemented his ideas with Perl::Critic. I don't think anyone should underestimate his contributions for improving Perl coding standards...

Re: Request for Perl::Critic Testimonials
by CountZero (Bishop) on Jul 16, 2006 at 20:30 UTC
    Your node caused me to install Perl::Critic, something I wanted to do already a long time.

    As I'm on Windows and using ActiveState Perl (5.8.7) I used ppm to install Perl::Critic and that install seemed to succeed.

    Unfortunately whenever I try to "critique" a file, it always crashes with a message such as "Not a CODE reference at C:/Data/webserver/Perl/site/lib/PPI/Statement/ line 86.".

    I have the latest version of PPI installed (version 1.115) and that install (through CPAN) and all tests went without a problem.

    Anyone knows a solution?


    "If you have four groups working on a compiler, you'll get a 4-pass compiler." - Conway's Law

      I am using Activestate Perl 5.8.8 with PPI 1.115, and Perl::Critic 0.17. I have no problems.

      The error does seem familiar however, I just cannot place my finger on it. Have you tried checking some other files or does it always return this error regardless of the file?

        As far as I could see, the error is fairly general and seems perhaps related to the checking of "pragma". At least, when I tried to re-install Perl::Critic with CPAN, the install gave an error when doing the "pragma" test.

        May be I should just switch to AS Perl 5.8.8 ... but I have tons of modules installed (some through ppm, some through CPAN) and it takes me days to re-install them all.


        "If you have four groups working on a compiler, you'll get a 4-pass compiler." - Conway's Law

Re: Request for Perl::Critic Testimonials
by diotalevi (Canon) on Jul 18, 2006 at 01:15 UTC

    Ever since starting to clean up some of my ~/bin to be Perl::Critic compliant I've found that I make more syntax errors and my code is harder to read. I suppose those two go together. There seem to be a small number of rules like ' ' -> q that seem designed just to make things difficult to parse. My tool progression went from having automatic syntax checks to automatic lint checks to automatic Perl::Critic tests. The transition away from things that actually compile the source started letting me leave dumb syntax bugs in my code. Bummer.

    I was already in a sweet spot where I only ever saved syntactically correct things. I might have other bugs but at least I'd always pass perl's compilation.

    I figure I'm going to have to either make P::C do Lint as well (which happens to include a syntax check) or uh... I dunno. Something. I figure having P::C + Lint is the best of both worlds because that's really P::C + Lint + Syntax. The only problem I have with Lint right now is that I haven't written in pragmas to disable it's checks in contexts where they're inappropriate.

    ⠤⠤ ⠙⠊⠕⠞⠁⠇⠑⠧⠊

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: perlmeditation [id://561416]
Approved by planetscape
Front-paged by gmax
and all is quiet...

How do I use this? | Other CB clients
Other Users?
Others having an uproarious good time at the Monastery: (3)
As of 2017-03-25 04:23 GMT
Find Nodes?
    Voting Booth?
    Should Pluto Get Its Planethood Back?

    Results (310 votes). Check out past polls.