Login
Holy smokes, it works!
authorJeffrey Ryan Thalhammer <jeff@imaginative-software.com>
Mon, 27 Oct 2008 05:44:56 +0000 (05:44 +0000)
committerJeffrey Ryan Thalhammer <jeff@imaginative-software.com>
Mon, 27 Oct 2008 05:44:56 +0000 (05:44 +0000)
Using the --warn-about-useless-no-critic option, I found several places
where we were disabling too many policies, or were disabling them
due to bugs that have since been fixed.  This is a pretty cool feature,
but I have reservations about making it into a regular Policy.  Since
we use pattern-matching to select the Policies, it is really easy for
someone to disable a *future* policy.  So I'm not sure I want to force
them to change their '## no critic' markers just because they've upgraded
P::C.

BTW, I discovered that we were using the wrong syntax in some of our
'## no critic' markers:

  ## no critic   'SomePolicy'  # wrong, disables all policies.
  ## no critic   (SomePolicy)  # ok
  ## no critic qw(SomePolicy)  # ok

We should probably emit a warning or something if we come across
a '## no critic' marker that looks wrong.

19 files changed:
bin/perlcritic
lib/Perl/Critic/Exception/AggregateConfiguration.pm
lib/Perl/Critic/Policy/BuiltinFunctions/ProhibitBooleanGrep.pm
lib/Perl/Critic/Policy/BuiltinFunctions/ProhibitReverseSortBlock.pm
lib/Perl/Critic/Policy/BuiltinFunctions/ProhibitUniversalCan.pm
lib/Perl/Critic/Policy/BuiltinFunctions/ProhibitUniversalIsa.pm
lib/Perl/Critic/Policy/InputOutput/ProhibitJoinedReadline.pm
lib/Perl/Critic/Policy/InputOutput/RequireBriefOpen.pm
lib/Perl/Critic/Policy/Miscellanea/RequireRcsKeywords.pm
lib/Perl/Critic/Policy/Modules/ProhibitEvilModules.pm
lib/Perl/Critic/Policy/RegularExpressions/ProhibitEnumeratedClasses.pm
lib/Perl/Critic/Policy/RegularExpressions/ProhibitEscapedMetacharacters.pm
lib/Perl/Critic/Policy/RegularExpressions/ProhibitUnusedCapture.pm
lib/Perl/Critic/Policy/Subroutines/ProhibitManyArgs.pm
lib/Perl/Critic/Policy/Subroutines/RequireArgUnpacking.pm
lib/Perl/Critic/Policy/Variables/ProhibitLocalVars.pm
lib/Perl/Critic/TestUtils.pm
lib/Perl/Critic/Utils/PPIRegexp.pm
lib/Perl/Critic/Violation.pm

index 9b07990..e4911eb 100755 (executable)
@@ -549,7 +549,7 @@ sub policy_docs {
     my @site_policies  = Perl::Critic::PolicyFactory->site_policy_names();
     my @matching_policies  = grep { $_ =~ m/$pattern/ixms } @site_policies;
 
-    # "-T" means don't send to pager   
+    # "-T" means don't send to pager
     my @perldoc_output = map {`perldoc -T $_`} @matching_policies;  ## no critic (ProhibitBacktick)
     out @perldoc_output;
 
index ace846d..c1e4eab 100644 (file)
@@ -73,7 +73,7 @@ sub add_exception_or_rethrow {
     my ( $self, $eval_error ) = @_;
 
     return if not $eval_error;
-    confess $eval_error if not ref $eval_error; ## no critic (RequireUseOfExceptions)
+    confess $eval_error if not ref $eval_error;
 
     if ( $eval_error->isa('Perl::Critic::Exception::Configuration') ) {
         $self->add_exception($eval_error);
@@ -84,7 +84,7 @@ sub add_exception_or_rethrow {
         $self->add_exceptions_from($eval_error);
     }
     else {
-        die $eval_error; ## no critic (RequireUseOfExceptions, RequireCarping)
+        die $eval_error; ## no critic (RequireCarping)
     }
 
     return;
index 69aa8d4..c56d14c 100644 (file)
@@ -77,7 +77,7 @@ sub _does_parent_cause_boolean {
     my $prev = $token->sprevious_sibling;
     return if $prev;
     my $parent = $token->statement->parent;
-    for (my $node = $parent; $node; $node = $node->parent) { ##no critic 'CStyleForLoop'
+    for (my $node = $parent; $node; $node = $node->parent) { ## no critic (CStyleForLoop)
         next if $node->isa('PPI::Structure::List');
         return 1 if $node->isa('PPI::Structure::Condition');
     }
index bfba053..83f34c8 100644 (file)
@@ -19,7 +19,7 @@ our $VERSION = '1.093_01';
 
 #-----------------------------------------------------------------------------
 
-Readonly::Scalar my $DESC => q{Forbid $b before $a in sort blocks}; ## no critic (Interpolation)
+Readonly::Scalar my $DESC => q{Forbid $b before $a in sort blocks}; ## no critic (InterpolationOfMetachars)
 Readonly::Scalar my $EXPL => [ 152 ];
 
 #-----------------------------------------------------------------------------
index 1444709..511c9db 100644 (file)
@@ -20,7 +20,7 @@ our $VERSION = '1.093_01';
 #-----------------------------------------------------------------------------
 
 Readonly::Scalar my $DESC => q{UNIVERSAL::can should not be used as a function};
-Readonly::Scalar my $EXPL => q{Use eval{$obj->can($pkg)} instead};  ##no critic 'RequireInterp';
+Readonly::Scalar my $EXPL => q{Use eval{$obj->can($pkg)} instead};  ## no critic (RequireInterp);
 
 #-----------------------------------------------------------------------------
 
index e174b8d..8b4386a 100644 (file)
@@ -20,7 +20,7 @@ our $VERSION = '1.093_01';
 #-----------------------------------------------------------------------------
 
 Readonly::Scalar my $DESC => q{UNIVERSAL::isa should not be used as a function};
-Readonly::Scalar my $EXPL => q{Use eval{$obj->isa($pkg)} instead};  ##no critic 'RequireInterp';
+Readonly::Scalar my $EXPL => q{Use eval{$obj->isa($pkg)} instead};  ## no critic (RequireInterp);
 
 #-----------------------------------------------------------------------------
 
index 9eee543..50258a9 100644 (file)
@@ -20,7 +20,7 @@ our $VERSION = '1.093_01';
 
 #-----------------------------------------------------------------------------
 
-Readonly::Scalar my $DESC => q{Use "local $/ = undef" or File::Slurp instead of joined readline}; ##no critic qw(Interpolation)
+Readonly::Scalar my $DESC => q{Use "local $/ = undef" or File::Slurp instead of joined readline}; ## no critic qw(InterpolationOfMetachars)
 Readonly::Scalar my $EXPL => [213];
 
 #-----------------------------------------------------------------------------
index f45ce7c..742904d 100644 (file)
@@ -25,7 +25,7 @@ our $VERSION = '1.093_01';
 Readonly::Scalar my $DESC => q<Close filehandles as soon as possible after opening them>;
 Readonly::Scalar my $EXPL => [209];
 
-Readonly::Scalar my $SCALAR_SIGIL => q<$>;  ## no critic (InterpolationOfLiterals)
+Readonly::Scalar my $SCALAR_SIGIL => q<$>;
 Readonly::Scalar my $GLOB_SIGIL   => q<*>;
 
 #-----------------------------------------------------------------------------
index c9c9ef3..5a2d4fa 100644 (file)
@@ -64,7 +64,6 @@ sub initialize_if_enabled {
     # Set configuration, if defined.
     my @keywords = keys %{ $self->{_keywords} };
     if ( @keywords ) {
-        ## no critic ProhibitEmptyQuotes
         $self->{_keyword_sets} = [ [ @keywords ] ];
     }
 
index 25b5b1e..05d36c9 100644 (file)
@@ -99,7 +99,7 @@ sub _parse_modules {
             # These are module name patterns (e.g. /Acme/)
             my $actual_regex;
 
-            eval { $actual_regex = qr/$regex_string/; 1 }  ## no critic (RegularExpressions)
+            eval { $actual_regex = qr/$regex_string/; 1 }  ## no critic (ExtendedFormatting, LineBoundaryMatching, DotMatchAnything)
                 or throw_policy_value
                     policy         => $self->get_short_name(),
                     option_name    => 'modules',
index 31378f6..96f357f 100644 (file)
@@ -29,7 +29,7 @@ Readonly::Scalar my $DESC => q{Use named character classes};
 Readonly::Scalar my $EXPL => [248];
 
 Readonly::Array my @PATTERNS => (  # order matters: most to least specific
-   [q{ },'\\t','\\r','\\n']      => ['\\s', '\\S'],  ##no critic (Interpolation)
+   [q{ },'\\t','\\r','\\n']      => ['\\s', '\\S'],  ## no critic (InterpolationOfMetachars)
    ['A-Z','a-z','_']             => ['\\w', '\\W'],
    ['A-Z','a-z']                 => ['[[:alpha:]]','[[:^alpha:]]'],
    ['A-Z']                       => ['[[:upper:]]','[[:^upper:]]'],
@@ -76,7 +76,7 @@ sub _get_character_class_violations {
     for my $element ($anyof->children) {
         if ($element->isa('Perl::Critic::PPIRegexp::exact')) {
             my @tokens = split m/(\\.[^\\]*)/xms, $element->content;
-            for my $token (map { split m/\A (\\[nrf])/xms, _fixup($_); } @tokens) {  ##no critic(Comma) ## TODO: FALSE POSITIVE
+            for my $token (map { split m/\A (\\[nrf])/xms, _fixup($_); } @tokens) {
                 $elements{$token} = 1;
             }
         } elsif ($element->isa('Perl::Critic::PPIRegexp::anyof_char') ||
@@ -111,9 +111,9 @@ sub _get_character_class_violations {
 }
 
 Readonly::Hash my %HEX => (  # Note: this is ASCII specific!
-   '0a' => '\\n',  ##no critic (Interpolation)
-   '0c' => '\\f',  ##no critic (Interpolation)
-   '0d' => '\\r',  ##no critic (Interpolation)
+   '0a' => '\\n',  ## no critic (InterpolationOfMetachars)
+   '0c' => '\\f',  ## no critic (InterpolationOfMetachars)
+   '0d' => '\\r',  ## no critic (InterpolationOfMetachars)
    '20' => q{ },
 );
 sub _fixup {
index 90d9529..07b7aff 100644 (file)
@@ -26,8 +26,7 @@ our $VERSION = '1.093_01';
 Readonly::Scalar my $DESC => q{Use character classes for literal metachars instead of escapes};
 Readonly::Scalar my $EXPL => [247];
 
-Readonly::Hash my %REGEXP_METACHARS =>
-    hashify split m/ /xms, '{ } ( ) . * + ? |'; ##no critic(Interpolation)
+Readonly::Hash my %REGEXP_METACHARS => hashify(split / /xms, '{ } ( ) . * + ? |');
 
 #-----------------------------------------------------------------------------
 
index 62fd95e..0c067a5 100644 (file)
@@ -87,7 +87,7 @@ sub violates {
     return $self->violation( $DESC, $EXPL, $elem );
 }
 
-sub _enough_assignments {  ##no critic(ExcessComplexity) # TODO
+sub _enough_assignments {
     my ($elem, $captures) = @_;
 
     # look backward for the assignment operator
index b43a05b..377c24e 100644 (file)
@@ -25,8 +25,8 @@ our $VERSION = '1.093_01';
 
 #-----------------------------------------------------------------------------
 
-Readonly::Scalar my $AT => q{@}; ##no critic(Interpolation)
-Readonly::Scalar my $AT_ARG => q{@_}; ##no critic(Interpolation)
+Readonly::Scalar my $AT => q{@};
+Readonly::Scalar my $AT_ARG => q{@_}; ## no critic (InterpolationOfMetachars)
 
 Readonly::Scalar my $DESC => q{Too many arguments};
 Readonly::Scalar my $EXPL => [182];
index 0ab88a5..3bd7112 100644 (file)
@@ -25,8 +25,8 @@ our $VERSION = '1.093_01';
 
 #-----------------------------------------------------------------------------
 
-Readonly::Scalar my $AT => q{@}; ##no critic(Interpolation)
-Readonly::Scalar my $AT_ARG => q{@_}; ##no critic(Interpolation)
+Readonly::Scalar my $AT => q{@};
+Readonly::Scalar my $AT_ARG => q{@_}; ## no critic (InterpolationOfMetachars)
 
 Readonly::Scalar my $DESC => qq{Always unpack $AT_ARG first};
 Readonly::Scalar my $EXPL => [178];
index 44b0a92..42cbf3d 100644 (file)
@@ -48,7 +48,7 @@ sub _all_global_vars {
     for my $variable_name ( $elem->variables() ) {
         next if $variable_name =~ $PACKAGE_RX;
         # special exception for Test::More
-        next if $variable_name eq '$TODO'; ##no critic(Interpolat)
+        next if $variable_name eq '$TODO'; ## no critic (InterpolationOfMetachars)
         return if ! is_perl_global( $variable_name );
     }
     return 1;
index 3563afd..6c8eb0c 100644 (file)
@@ -195,7 +195,7 @@ sub _subtests_from_file {
     while ( <$fh> ) {
         ++$lineno;
         chomp;
-        my $inheader = /^## name/ .. /^## cut/; ## no critic(RegularExpression)
+        my $inheader = /^## name/ .. /^## cut/; ## no critic (ExtendedFormatting LineBoundaryMatching DotMatchAnything)
 
         my $line = $_;
 
@@ -273,7 +273,7 @@ sub _finalize_subtest {
 
     if (defined $subtest->{error}) {
         if ( $subtest->{error} =~ m{ \A / (.*) / \z }xms) {
-            $subtest->{error} = eval {qr/$1/}; ##no critic (RegularExpressions::)
+            $subtest->{error} = eval {qr/$1/}; ## no critic (ExtendedFormatting LineBoundaryMatching DotMatchAnything)
             if ($EVAL_ERROR) {
                 throw_internal
                     "$subtest->{name} 'error' has a malformed regular expression";
index 8191acc..c852bb4 100644 (file)
@@ -131,11 +131,11 @@ sub get_delimiters {
             my $src_isa_name = $src_class . '::ISA';
             my $dest_isa_name = $dest_class . '::ISA';
             my @isa;
-            for my $isa (eval "\@$src_isa_name") { ##no critic(Eval)
+            for my $isa (eval "\@$src_isa_name") { ## no critic (StringyEval)
                 my $dest_isa = _get_ppi_package($isa, $re_node);
                 push @isa, $dest_isa;
             }
-            eval "\@$dest_isa_name = qw(@isa)"; ##no critic(Eval)
+            eval "\@$dest_isa_name = qw(@isa)"; ## no critic (Eval)
             croak $EVAL_ERROR if $EVAL_ERROR;
         }
         return $dest_class;
index b5590f1..7b653cb 100644 (file)
@@ -87,7 +87,6 @@ sub sort_by_location {  ##no critic(ArgUnpacking)
     ref $_[0] || shift;              #Can call as object or class method
     return scalar @_ if ! wantarray; #In case we are called in scalar context
 
-    ## no critic qw(RequireSimpleSort);
     ## TODO: What if $a and $b are not Violation objects?
     return
         map {$_->[0]}
@@ -103,7 +102,6 @@ sub sort_by_severity {  ##no critic(ArgUnpacking)
     ref $_[0] || shift;              #Can call as object or class method
     return scalar @_ if ! wantarray; #In case we are called in scalar context
 
-    ## no critic qw(RequireSimpleSort);
     ## TODO: What if $a and $b are not Violation objects?
     return
         map {$_->[0]}