Skip Menu |

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Perl-Critic CPAN distribution.

Report information
The Basics
Id: 59176
Status: open
Priority: 0/
Queue: Perl-Critic
People
Owner: Nobody in particular
Requestors: user42 [...] zip.com.au
Cc:
AdminCc:
Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)

History Show all quoted text
  Wed Jul 07 18:07:30 2010 user42 [...] zip.com.au - Ticket created
Subject: "no critic" for pod after __END__
Date: Thu, 08 Jul 2010 08:07:02 +1000
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
Foo.pm below critiqued as "perlcritic -1 Foo.pm" gets violation (among several others), Pod NAME does not match the package declaration at line 5, column 1. (no explanation). (Severity: 1) where I hoped the "no critic" directive in Foo.pm would suppress that policy. It seems to suppress successfully if the __END__ is removed, but I hoped it would be possible to put pod after __END__ and still affect pod policies on it. I don't mind if the no critic directive has to come before __END__ to be seen, though you'd be tempted to grep them out after an __END__ too.
## no critic (RequirePackageMatchesPodName) __END__ =head1 NAME Bar - wrong module name here!
  Thu Jul 08 20:18:09 2010 wyant [...] cpan.org - Correspondence added
Patch committed as SVN revision 3880, to have Perl::Critic::Annotation objects extend their end ranges into the __END__ of the document. The ## no critic must come at the beginning of the document, because the policy applies to the whole document.
  Thu Jul 08 20:18:10 2010 The RT System itself - Status changed from 'new' to 'open'
  Thu Jul 22 21:10:23 2010 user42 [...] zip.com.au - Correspondence added
Subject: Re: [rt.cpan.org #59176] "no critic" for pod after __END__
Date: Fri, 23 Jul 2010 11:10:04 +1000
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
"Tom Wyant via RT" <bug-Perl-Critic@rt.cpan.org> writes: Show quoted text
> > Patch committed as SVN revision 3880, to have Perl::Critic::Annotation > objects extend their end ranges into the __END__ of the document.
I'm not sure it's right on the Foo.pm I posted. Try one without a final "=cut", so the pod to be affected is the last element. I wonder if the $ending_line should allow for elements like pods which have newlines in them.
  Fri Jul 23 10:23:10 2010 wyant [...] cpan.org - Correspondence added
On Thu Jul 22 21:10:23 2010, user42@zip.com.au wrote: Show quoted text
> > I'm not sure it's right on the Foo.pm I posted.
In my Perl-Critic development directory, I just now did $ perl -Mblib bin/perlcritic --single RequirePackageMatchesPodName ../Foo.pm (../Foo.pm being a fresh download of the file you attached to the original ticket) and found no violation. Show quoted text
> Try one without a final > "=cut", so the pod to be affected is the last element.
I also tried this with the attached Foo2.pm (no '=cut'), and got no violation. Can you post an example that the patch does not deal with, and say how it fails to deal with it? Show quoted text
> I wonder if the $ending_line should allow for elements like pods > which have newlines in them.
I believe not. Perl::Critic does not analyze Perl directly, but instead analyzes a PPI parse. The 'effective range' calculation is based on the line PPI reports for the element being analyzed. If you run the attached Foo2.pm file through ppidump (found in the 'tools' directory of the Perl::Critic kit) you will see that the lump of POD at the end all ends up in a single PPI::Token::Pod object.
Subject: Foo2.pm
package Foo2; 1; ## no critic (RequirePackageMatchesPodName) __END__ =pod =head1 NAME Bar - This is the wrong name.
  Fri Jul 23 19:09:54 2010 user42 [...] zip.com.au - Correspondence added
Subject: Re: [rt.cpan.org #59176] "no critic" for pod after __END__
Date: Sat, 24 Jul 2010 09:09:37 +1000
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
"Tom Wyant via RT" <bug-Perl-Critic@rt.cpan.org> writes: Show quoted text
> > no violation.
Ah, yeah, sorry about that, I realized shortly after posting that I'd probably done it to myself. I've got it jigged up to report the violation at the actual offending "=head NAME" line number, per below (exercised a bit, not really tested beyond that :). I forget if I've submitted it yet. Show quoted text
> line PPI reports for the element being analyzed.
Oh, well, you don't want to limit reports to the first line of an element. It should be possible to pinpoint offending positions in big pod blocks or here documents and perhaps big literal strings. The way ppi generates its line/column is a bit awful, and you'd probably like it to help working out an offset into an element, but even without that getting close to the right linenum in a pod block isn't too hard.
Index: RequirePackageMatchesPodName.pm =================================================================== --- RequirePackageMatchesPodName.pm (revision 3881) +++ RequirePackageMatchesPodName.pm (working copy) @@ -52,6 +52,7 @@ next if $content !~ m{^=head1 [ \t]+ NAME [ \t]*$ \s*}cgxms; my ($pod_pkg) = $content =~ m{\G (\S+) }cgxms; + my $pkg_pos = pos($content); if (!$pod_pkg) { return $self->violation( $DESC, q{Empty name declaration}, $elem ); @@ -67,12 +68,23 @@ return if $pkg eq $pod_pkg; } - return $self->violation( $DESC, $EXPL, $pod ); + my $violation = $self->violation( $DESC, $EXPL, $pod ); + $violation->set_line_number_offset + (_str_pos_to_linenum ($content, $pkg_pos) - 1); + return $violation; + } return; # no NAME section found } +# returning 1 for first line +sub _str_pos_to_linenum { + my ($str, $pos) = @_; + $str = substr ($str, 0, $pos); + return scalar($str =~ tr/\n/x/) + 1; +} + 1; __END__
Index: Violation.pm =================================================================== --- Violation.pm (revision 3881) +++ Violation.pm (working copy) @@ -128,6 +128,34 @@ #----------------------------------------------------------------------------- +sub set_line_number_offset { + my ($self, $offset) = @_; + + my @location = @{$self->{_location}}; + $location[$LOCATION_LINE_NUMBER] += $offset; + $location[$LOCATION_LOGICAL_LINE_NUMBER] += $offset; + $location[$LOCATION_COLUMN_NUMBER] = 1; + $location[$LOCATION_VISUAL_COLUMN_NUMBER] = 1; + $self->{_location} = \@location; + + # Setup $self->source() to give the actual offending line at $offset. + # This may be a long way from the start of statement etc for a big pod or + # here document. + # + my $elem = $self->{_elem}; + my $str = (defined $elem ? $elem->document->content : $EMPTY); + $self->{_source} = _str_line_n ($str, $location[$LOCATION_LINE_NUMBER]); +} + +# counting from $n==1 for the first line +sub _str_line_n { + my ($str, $n) = @_; + $n--; + return ($str =~ /^(.*\n){$n}(.*)/ ? $2 : $EMPTY); +} + +#----------------------------------------------------------------------------- + sub location { my $self = shift;
  Tue Oct 19 21:14:24 2010 wyant [...] cpan.org - Correspondence added
A hack to the violation message was made a while back (as SVN revision 3891) to add the line number to the text of the error message. This was the minimal change, and has precedent in other Documentation policies. But it produces a somewhat confusing error message, since there are two line numbers. We appreciate the proffered patch. At the moment, though, we're investigating making Perl::Critic::Policy->violation() and Perl::Critic::Violation->new() understand named arguments. If this works out, it would make this and a couple other tickets a snap, without having to retrieve and modify the Violation object after it was created. NOTE TO RT QUEUE MAINTAINERS I have marked this patched, but we may want to hold this open until either the named argument effort collapses or until it is implemented and we can redo the patch using named arguments.
  Tue Oct 19 21:14:25 2010 wyant [...] cpan.org - Status changed from 'open' to 'patched'
  Sat Oct 23 20:00:46 2010 user42 [...] zip.com.au - Correspondence added
Subject: Re: [rt.cpan.org #59176] "no critic" for pod after __END__
Date: Sun, 24 Oct 2010 11:00:24 +1100
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
"Tom Wyant via RT" <bug-Perl-Critic@rt.cpan.org> writes: Show quoted text
> > Perl::Critic::Violation->new() understand named arguments.
There'd be room at the end for options. There'd could be more than one way to do it when expressing an offset into an element. An $self->violation ($desc, $detail, $elem, offset_linenum => $linenum); would suit pod crunched stuff, or something like offset_in_string => $charnum would be good for PPI::Token::Quote and PPI::Token::QuoteLike elements expressing a position within the string payload as such, skipping whatever ' or q{ quoting (including comments between the "q" and "{"). The QuoteLikes could get some help from PPI for where the payload begins in the full content. Pinpointing a string position will be good for long strings or objections to subtle things like RequireInterpolationOfMetachars where it's often not easy to tell by eye exactly where the problem is. I might even have opened a ticket for that already, ages ago.
  Sat Oct 23 20:00:47 2010 The RT System itself - Status changed from 'patched' to 'open'
  Sun Oct 24 12:33:04 2010 wyant [...] cpan.org - Correspondence added
On Sat Oct 23 20:00:46 2010, user42@zip.com.au wrote: Show quoted text
> "Tom Wyant via RT" <bug-Perl-Critic@rt.cpan.org> writes:
> > > > Perl::Critic::Violation->new() understand named arguments.
> > There'd be room at the end for options. There'd could be more than one > way to do it when expressing an offset into an element. An > > $self->violation ($desc, $detail, $elem, > offset_linenum => $linenum); > > would suit pod crunched stuff, or something like > > offset_in_string => $charnum > > would be good for PPI::Token::Quote and PPI::Token::QuoteLike elements > expressing a position within the string payload as such, skipping > whatever ' or q{ quoting (including comments between the "q" and "{"). > The QuoteLikes could get some help from PPI for where the payload begins > in the full content. > > Pinpointing a string position will be good for long strings or > objections to subtle things like RequireInterpolationOfMetachars where > it's often not easy to tell by eye exactly where the problem is. > I might even have opened a ticket for that already, ages ago.
As of right now the thinking is that rather than having a hybrid calling sequence (the polite term!) we go with a full named interface to Perl::Critic::Violation->new(). The positional interface stays for a while at least, but all new functionality goes into the named interface. The new arguments are going to be something like '-line-offset' and '-character', with either or both being allowed. The leading-dash convention appears to be deeply-enough entrenched in Perl::Critic that we're going to have to go with it. For various reasons, the Perl::Critic::Policy violation() method remains positional, with new method make_violation() having the named interface. But it may take a while for all this to come to fruition, because there's a release in the works and we don't want to upset things too badly until it happens. Then installing named arguments looks like a separate activity in itself. Once they're in, work on customizing the reported position can be done.
  Sun Oct 24 18:00:59 2010 wyant [...] cpan.org - Status changed from 'open' to 'patched'
  Fri Oct 29 00:14:52 2010 user42 [...] zip.com.au - Correspondence added
Subject: Re: [rt.cpan.org #59176] "no critic" for pod after __END__
Date: Fri, 29 Oct 2010 09:46:10 +1100
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
"Tom Wyant via RT" <bug-Perl-Critic@rt.cpan.org> writes: Show quoted text
> > hybrid calling sequence
If the first three args are always wanted and most of the time all that's needed then that style can be retained, and just have optional key/value args after them for tweaking.
  Fri Oct 29 00:14:53 2010 The RT System itself - Status changed from 'patched' to 'open'