Perl::Critic::TODO - Things for Perl::Critic developers to do
#######################################################################
# $URL: http://perlcritic.tigris.org/svn/perlcritic/trunk/Perl-Critic/TODO.pod $
# $Date: 2008-07-03 09:36:05 -0500 (Thu, 03 Jul 2008) $
# $Author: clonezone $
# $Revision: 2487 $
#######################################################################
Perl-Critic-More is a separate distribution for less-widely-accepted policies. It contains its own TODO.pod.
Just print out file names. I could have used this at work when combined with
--single-policy.
gvim `perlcritic --single-policy QuotedWordLists -l`
Requires ## no critic to take an argument:
## no critic (SomePolicyPattern) # ok ## no critic # not ok
Can't be done as a regular Policy because any line that violated it would disable it.
#line 123 "filename" directives.
For code generators and template languages that allow inline Perl code.
Yes, somebody has an in-house templating system where they've written a custom test module that extracts the perl code from a template and critiques it.
Actually, this would be useful for programs: Module::Build "fixes" shebang
lines so that there's the bit about invoking perl if the program is attempted
to be run by a Bourne shell, which throws the line numbers off when using
Test::P::C on the contents of a blib directory.
- Blank line count
- POD line count
- Comment line count
- Data section count
say as equivalent to print.
For example, the self compliance test now depends upon a Policy in the More distribution.
Something like using a "+" sign in front of the Policy name in its
configuration block, analogous to the "-" sign used for disabling a policy,
e.g. "[+Example::Policy]".
Don't allow compound names with forbidden words, like "last_record". Allow forbidden words in RHS of variable declarations
Also, we should make it easeir to add (or delete) words from the forbbiden list.
$pkg->_foo() because it can't tell the
difference between that and $self->_foo()
This should not complain about using warn or die if it's not in a
function, or if it's in main::.
Also, should allow die when it is obvious that the "message" is a reference.
Allow this construct:
for ( ... ) {
next unless /(....)/;
if ( $1 ) {
....
}
}
Right now, P::C thinks that the $1 isn't legal to use because it's
"outside" of the match. The thing is, we can only get to the if
if the regex matched.
while ( $str =~ /(expression)/ )
scalar( something, something ).
=head1 NAME POD section with content that matches
\A \s* [\w:]+ \s+ - \s+ \S. The single hyphen is the important bit. Also,
must be a single line.
Prevent $@ from being cleared unexpectedly by DESTROY methods.
package Foo;
sub DESTROY {
die "Died in Foo::DESTROY()";
}
package main;
eval {
my $foo = Foo->new();
die "Died in eval."
}
print $@; # "Died in Foo::DESTROY()", not "Died in eval.".
use statement to have an explicit import list. You could
still get around this by calling import directly.
use to have an explicitly empty import list. This is for
folks who like to see fully-qualified function names. Should probably provide
a list of exempt modules (like FindBin);
do "foo.pl". Not sure about this policy name.
use vars qw(...) and require our $foo instead. This
contradicts Miscellanea::Prohibit5006isms. Maybe verify use 5.6
before applying this policy. Low severity.
Foo::Bar->new, Bad: new Foo::Bar
split instead of m/\s*\w*\s*\w*\s*/. From MJD's Red Flags.
Any =headN and =over sections must not be empty. This helps catch
boilerplate (althought Test::Pod should catch empty =over blocks).
On the other hand, =item ... sections can be empty, since the item label is
content.
Complain about copy-and-paste code or docs from h2xs, Module::Starter::*, etc.
Here's a non-PPI implementation: http://search.cpan.org/src/JJORE/Carp-Clan-5.8/t/04boilerplate.t
if (scalar @array) be rewritten as if (@array).
Ban naked srtings as regexps, like:
print 1 if $str =~ $regexp;
Instead, it should be:
print 1 if $str =~ m/$regexp/;
or
print 1 if $str =~ m/$regexp/xms;
Ensure that the argument to a stringy eval is not a constant string. That's just wasteful. Real world examples include:
eval 'use Optional::Module';
which is better written as
eval { require Optional::Module; Optional::Module->import };
for performance gains and compile-time syntax checking.
Complain if user puts a backslash escape in front of non-special characters. For example:
m/\!/;
Make exceptions for \", \' and \` since those are often inserted to
workaround bugs in syntax highlighting.
Note that this is different inside character classes, where only ^, ]
and - need to be escaped, I think. Caret only needs to be escaped at the
beginning, and dash does NOT need to be escaped at the beginning and end. See
perlreref.
http://use.perl.org/~stu42j/journal/36412
Just about everyone has been bitten by if ($x = 10) { ... } when they meant
to use ==. A safer style is 10 == $x because omitting the second =
yields a noisy compile-time failure instead of silent runtime error.
ProhibitAssigmentInConditional complains if the condition of a while, until,
if or unless is solely an assignment. If it's anything more complex (like
if (($x=10)){} or while ($x=$y=$z){}), there is no warning.
RequireConstantBeforeEquals complains if the left side of an == is a
variable while the right side is a constant.
RequireConstantBeforeOperator complains if the left side of any comparison
operator (==, eq, <, etc) is a variable while the right side is a
constant.
exit() in files that lack a shebang. Inspired by
http://use.perl.org/~Ovid/journal/36746 and an analgous checker in
FindBugs.
All the code that deals with finding all the '##no critic' comments and noting which policies are disabled at each line seems like it would be better placed in Perl::Critic::Document. P::C::Document could then provide methods to indicate if a policy is disabled at a particular line. So the basic algorithm in Perl::Critic might look something like this:
foreach $element (@PPI_ELEMENTS) {
foreach $policy (@POLICIES) {
$line = $element->location->[0];
next if $doc->policy_is_disabled_at_line( $policy, $line );
push @violations, $policy->violates( $elem, $doc );
}
}
##no critic
##no critic and forgetting to do a
##use critic after the problematic section. Perhaps an option to
perlcritic to scan for such things is in order.
-cache flag to bin/perlcritic
If enabled, this turns on PPI::Cache:
require PPI::Cache;
my $cache_path = "/tmp/test-perl-critic-cache-$ENV{USER}";
mkdir $cache_path, oct 700 if (! -d $cache_path);
PPI::Cache->import(path => $cache_path);
This cachedir should perhaps include the PPI version number! At least until PPI incorporates its own version number in the cache.
(see t/40_criticize.t for a more robust implementation)
List::MoreUtils::any function.
In several places, Perl::Critic uses List::MoreUtils::any to see if
a string is a member of a list. Instead, I suggest using a named
subroutine that does a hash-lookup like this:
my %logical_ops = hashify( qw( ! || && ||= &&= and or not ) );
sub is_logical_op { return exists $logical_ops{ $_[0] }; }
Why?
http://rt.cpan.org/Ticket/Display.html?id=30140
ack now supports this.
We're waiting on the following bugs to get fixed in a CPAN release of PPI:
PPI does not preserve newlines. That makes CodeLayout::RequireConsistentNewlines impossible to implement under PPI. For now, it's implemented by pulling the source out of the file and skipping PPI.
It's unlikely that PPI will support mixde newlines anytime soon.