Login
First attempt to warn users when they have an unecessary "## no critic"
authorJeffrey Ryan Thalhammer <jeff@imaginative-software.com>
Sun, 26 Oct 2008 20:13:00 +0000 (20:13 +0000)
committerJeffrey Ryan Thalhammer <jeff@imaginative-software.com>
Sun, 26 Oct 2008 20:13:00 +0000 (20:13 +0000)
marker in their code.  I'm not happy with the option-name that I chose.
Also, I know there are bugs related to the shebang line, and the borders of
##no critic -> ##use critic blocks.

bin/perlcritic
lib/Perl/Critic.pm
lib/Perl/Critic/Config.pm
lib/Perl/Critic/Document.pm
lib/Perl/Critic/OptionsProcessor.pm
t/03_useless_pragmas.t [new file with mode: 0644]

index 195a022..1ac7bfe 100755 (executable)
@@ -454,6 +454,7 @@ sub _get_option_specification {
         theme=s
         top:i
         verbose=s
+        warn-about-useless-no-critic!
     );
 }
 
index fab273f..7555800 100644 (file)
@@ -158,6 +158,12 @@ sub _gather_violations {
     my @policies = $self->config->policies();
     my @violations = map { _critique($_, $doc) } @policies;
 
+    # Warn about unecessary "## no critic" markers
+    if ( $self->config->warn_about_useless_no_critic() ) {
+        my @warnings = $doc->useless_no_critic_warnings(@violations);
+        for (@warnings) { warn "$_\n"; }
+    }
+    
     # Accumulate statistics
     $self->statistics->accumulate( $doc, \@violations );
 
index 0bd7937..a03ab8a 100644 (file)
@@ -99,6 +99,10 @@ sub _init {
         $self->{_color} = boolean_to_number( dor( $args{-color}, $options_processor->color() ) );
         $self->{_criticism_fatal} =
           boolean_to_number(dor( $args{'-criticism-fatal'}, $options_processor->criticism_fatal() ) );
+          
+        $self->{_warn_about_useless_no_critic} =
+            boolean_to_number(dor( $args{'-warn-about-useless-no-critc'},
+                 $options_processor->warn_about_useless_no_critic() ) );
     }
 
     $self->_validate_and_save_theme($args{-theme}, $errors);
@@ -765,6 +769,13 @@ sub criticism_fatal {
 
 #-----------------------------------------------------------------------------
 
+sub warn_about_useless_no_critic {
+    my $self = shift;
+    return $self->{_warn_about_useless_no_critic};
+}
+
+#-----------------------------------------------------------------------------
+
 sub site_policy_names {
     return Perl::Critic::PolicyFactory::site_policy_names();
 }
@@ -973,6 +984,10 @@ Returns the value of the C<-pager> attribute for this Config.
 
 Returns the value of the C<-criticsm-fatal> attribute for this Config.
 
+=item C< warn_about_useless_no_critic() >
+
+Returns the value of the C<-warn-about-useless-no-critic> attribute for this Config.
+
 =back
 
 =head1 SUBROUTINES
index 4b41b3a..99842c6 100644 (file)
@@ -184,6 +184,40 @@ sub is_line_disabled {
 
 #-----------------------------------------------------------------------------
 
+sub useless_no_critic_warnings {
+    my ($self, @violations) = @_;
+    
+    
+    my %violation_lines = ();
+    for my $violation (@violations) {
+        my $line = $violation->location()->[0];
+        my $policy_name = $violation->policy();
+        $violation_lines{$policy_name}->{$line} = 1;
+    }
+    
+    
+    my @warnings = ();
+    my $file = $self->filename() || 'UNKNOWN';
+    
+    my %disabled_lines = %{ $self->{_disabled_lines} };
+    for my $line (keys %disabled_lines) {
+        my %disabled_policies = %{ $disabled_lines{$line} };
+        for my $policy_name (keys %disabled_policies) {
+            
+            if ($policy_name eq 'ALL' && not exists $violation_lines{$line}) {
+                push @warnings, qq{Useless disabling of all Policies in file "$file" at line $line.};
+            }
+            elsif (not $violation_lines{$line}->{$policy_name}) {
+                push @warnings, qq{Useless disabling of $policy_name in file "$file" at line $line.};
+            }
+        }
+    }
+    
+    return @warnings;
+}
+
+#-----------------------------------------------------------------------------
+
 sub _is_a_version_statement {
     my (undef, $element) = @_;
 
@@ -506,11 +540,20 @@ an internal table of which of the listed C<@policy_names> have
 been disabled at each line.  Returns C<$self>.
 
 
-=item C<< line_is_disabled($line, $policy_name) >>
+=item C<< is_line_disabled($line, $policy_name) >>
 
 Returns true if the given C<$policy_name> has been disabled for
 at C<$line> in this document.  Otherwise, returns false.
 
+=item C<< useless_no_critic_warnings(@violations) >>
+
+Given a list of violation objects that are assumed to have been found
+in this Document, returns a warning message for each line where a 
+policy was disabled using a C<"##no critic"> pseudo-pragma, but
+no violation was actually found on that line.  If multiple policies
+are disabled on a given line, then you'll get a warning message
+for each policy.
+
 
 =back
 
index dfa80df..d6f0978 100644 (file)
@@ -44,17 +44,19 @@ sub _init {
     $self->{_include}    = [ words_from_string( $include ) ];
 
     # Single-value defaults
-    $self->{_force}          = dor(delete $args{force},              $FALSE);
-    $self->{_only}           = dor(delete $args{only},               $FALSE);
+    $self->{_force}           = dor(delete $args{force},              $FALSE);
+    $self->{_only}            = dor(delete $args{only},               $FALSE);
     $self->{_profile_strictness} =
         dor(delete $args{'profile-strictness'}, $PROFILE_STRICTNESS_DEFAULT);
-    $self->{_single_policy}  = dor(delete $args{'single-policy'},    $EMPTY);
-    $self->{_severity}       = dor(delete $args{severity},           $SEVERITY_HIGHEST);
-    $self->{_theme}          = dor(delete $args{theme},              $EMPTY);
-    $self->{_top}            = dor(delete $args{top},                $FALSE);
-    $self->{_verbose}        = dor(delete $args{verbose},            $DEFAULT_VERBOSITY);
-    $self->{_criticism_fatal} = dor(delete $args{'criticism-fatal'}, $FALSE);
-    $self->{_pager}          = dor(delete $args{pager},              $EMPTY);
+    $self->{_single_policy}   = dor(delete $args{'single-policy'},    $EMPTY);
+    $self->{_severity}        = dor(delete $args{severity},           $SEVERITY_HIGHEST);
+    $self->{_theme}           = dor(delete $args{theme},              $EMPTY);
+    $self->{_top}             = dor(delete $args{top},                $FALSE);
+    $self->{_verbose}         = dor(delete $args{verbose},            $DEFAULT_VERBOSITY);
+    $self->{_criticism_fatal} = dor(delete $args{'criticism-fatal'},  $FALSE);
+    $self->{_warn_about_useless_no_critic} = 
+        dor(delete $args{'warn_about_useless_no_critic'}, $FALSE);
+    $self->{_pager}           = dor(delete $args{pager},              $EMPTY);
 
     # If we're using a pager or not outputing to a tty don't use colors.
     # Can't use IO::Interactive here because we /don't/ want to check STDIN.
@@ -169,6 +171,13 @@ sub criticism_fatal {
 
 #-----------------------------------------------------------------------------
 
+sub warn_about_useless_no_critic {
+    my ($self) = @_;
+    return $self->{_warn_about_useless_no_critic};
+}
+
+#-----------------------------------------------------------------------------
+
 sub force {
     my ($self) = @_;
     return $self->{_force};
@@ -294,6 +303,9 @@ command string).
 
 Returns the default C<criticism-fatal> setting (Either 1 or 0).
 
+=item C< warn_about_useless_no_critic() >
+
+Returns the default C<warn-about-useless-no-critic> setting (Either 1 or 0).
 
 =back
 
diff --git a/t/03_useless_pragmas.t b/t/03_useless_pragmas.t
new file mode 100644 (file)
index 0000000..9d241ac
--- /dev/null
@@ -0,0 +1,51 @@
+#!perl
+
+##############################################################################
+#     $URL$
+#    $Date$
+#   $Author$
+# $Revision$
+##############################################################################
+
+use 5.006001;
+use strict;
+use warnings;
+
+use Test::More (tests => 1);
+use Perl::Critic::PolicyFactory (test => 1);
+use Perl::Critic::Document;
+use PPI::Document;
+
+#-----------------------------------------------------------------------------
+
+my $violation_free_code = <<'END_PERL';
+
+$foo = 0;  ## this line is not disabled
+
+## no critic;    #\
+$foo = 1;        # |-> this block counts as 3 disabled lines
+## use critic;   #/
+
+$foo = 2; ## no critic;
+
+$foo = 3; ## no critic (MagicNumbers)
+
+sub foo {  ## no critic (ExcessComplexity, BuiltinHomonyms)
+    return 1;
+}
+
+$foo = 5;  ## this line is not disabled
+
+END_PERL
+
+my $ppi_doc = PPI::Document->new(\$violation_free_code);
+my $doc = Perl::Critic::Document->new($ppi_doc);
+
+my @site_policies = Perl::Critic::PolicyFactory::site_policy_names();
+$doc->mark_disabled_lines(@site_policies);
+
+my @empty_violation_list = ();
+my @got_warnings = $doc->useless_no_critic_warnings(@empty_violation_list);
+is(scalar @got_warnings, 7, 'Got correct numer of useless-no-critic warnings.');
+
+#-----------------------------------------------------------------------------