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:
33936
Status:
rejected
Priority:
Low/Low
Queue:

People
Owner:
Nobody in particular
Requestors:
perl [...] galumph.com
Cc:
AdminCc:

BugTracker
Severity:
(no value)
Broken in:
(no value)
Fixed in:
(no value)



CC: Heiko <heiko@hexco.de>
Subject: ProhibitNestedSubs doesn't handle subs declared inside of eval/scheduled blocks.
Date: Sun, 09 Mar 2008 16:55:49 -0500
To: bug-Perl-Critic@rt.cpan.org
From: Elliot Shank <perl@galumph.com>
Subroutines::ProhibitNestedSubs gives Nested named subroutine at line 12, column 9. Declaring a named sub inside another named sub does not prevent the inner sub from being global.. (Severity: 5) on this code: sub BEGIN { eval { sub color }; } The purpose is to make color() a no-op under certain runtime situations. I think BEGIN (and possibly CHECK and INIT ) subroutines should be exceptions to this rule. The patch makes an exception for BEGIN subroutines.
--- ProhibitNestedSubs.pm.org 2008-02-25 22:14:31.265625000 +0100 +++ ProhibitNestedSubs.pm.new 2008-02-23 22:58:08.390625000 +0100 @@ -11,7 +11,7 @@ use warnings; use Readonly; -use Perl::Critic::Utils qw{ :severities }; +use Perl::Critic::Utils qw{ :severities :classification }; use base 'Perl::Critic::Policy'; our $VERSION = '1.080'; @@ -38,6 +38,13 @@ my $inner = $elem->find_first('PPI::Statement::Sub'); return if not $inner; + my $barewords = $elem->find('PPI::Token::Word'); + + return if (2 <= scalar @{ $barewords } + && $barewords->[0]->{'content'} eq 'sub' + && is_subroutine_name( $barewords->[1] ) + && $barewords->[1]->{'content'} eq 'BEGIN'); + # Must be a violation... return $self->violation($DESC, $EXPL, $inner); }
RT-Send-CC: heiko@hexco.de
On Sun Mar 09 17:56:08 2008, clonezone wrote:
Show quoted text
> Subroutines::ProhibitNestedSubs gives > > Nested named subroutine at line 12, column 9. Declaring a named sub > inside another named sub does not prevent the inner sub from being > global.. (Severity: 5) > > on this code: > sub BEGIN > { > eval { sub color }; > } > > The purpose is to make color() a no-op under certain runtime > situations. I think BEGIN (and possibly CHECK and INIT ) subroutines > should be exceptions to this rule.
I don't understand what's supposed to be going on here. What is the purpose of wrapping "sub color" in an eval block? Is color() declared later in this package, or was it declared in a package that had already been loaded? Is color() going to get called at compile-time or run-time (or both)? I guess if I wanted to override a subroutine at run-time I would probably do something like this: *color = sub {...}; And if I wanted to override it at compile-time, then I would wrap it in a BEGIN block just as you did: BEGIN { *color = sub {...} } In any event, it looks like your doing something that's quite out of the ordinary. In which case, the "## no critic" flag is a way to communicate to your colleagues that you know what you're doing. But please enlighten me if I've misunderstood your intention. -Jeff
CC: <heiko@hexco.de>
Subject: AW: [rt.cpan.org #33936] ProhibitNestedSubs doesn't handle subs declared inside of eval/scheduled blocks.
Date: Mon, 28 Jul 2008 09:05:40 +0200
To: <bug-Perl-Critic@rt.cpan.org>
From: "Eissfeldt, Heiko" <heiko.eissfeldt@siemens.com>
Show quoted text
-----Ursprüngliche Nachricht----- Von: Jeffrey Ryan Thalhammer via RT [mailto:bug-Perl-Critic@rt.cpan.org] Gesendet: Sonntag, 27. Juli 2008 07:01 Cc: heiko@hexco.de Betreff: [rt.cpan.org #33936] ProhibitNestedSubs doesn't handle subs declared inside of eval/scheduled blocks. <URL: http://rt.cpan.org/Ticket/Display.html?id=33936 > On Sun Mar 09 17:56:08 2008, clonezone wrote:
> Subroutines::ProhibitNestedSubs gives > > Nested named subroutine at line 12, column 9. Declaring a named sub > inside another named sub does not prevent the inner sub from being > global.. (Severity: 5) > > on this code: > sub BEGIN > { > eval { sub color }; > } > > The purpose is to make color() a no-op under certain runtime > situations. I think BEGIN (and possibly CHECK and INIT ) subroutines > should be exceptions to this rule.
>I don't understand what's supposed to be going on here. What is the >purpose of wrapping "sub color" in an eval block? Is color() declared >later in this package, or was it declared in a package that had already >been loaded? Is color() going to get called at compile-time or run-time >(or both)?
>I guess if I wanted to override a subroutine at run-time I would >probably do something like this:
> *color = sub {...};
>And if I wanted to override it at compile-time, then I would wrap it in >a BEGIN block just as you did:
> BEGIN { *color = sub {...} }
Yes, thanks, that is another (probably better) solution.
>In any event, it looks like your doing something that's quite out of the >ordinary. In which case, the "## no critic" flag is a way to >communicate to your colleagues that you know what you're doing.
>But please enlighten me if I've misunderstood your intention.
My intention was to define a no-op sub 'color' as a fallback, if the supplying module could not be loaded or was not available. So the rest of the app code did not need to change. Thanks for your feedback. Heiko aka Hexcoder
Subject: Re: AW: [rt.cpan.org #33936] ProhibitNestedSubs doesn't handle subs declared inside of eval/scheduled blocks.
Date: Mon, 28 Jul 2008 21:21:17 -0700
To: bug-Perl-Critic@rt.cpan.org
From: Jeffrey Thalhammer <jeff@imaginative-software.com>

On Jul 28, 2008, at 12:06 AM, Eissfeldt, Heiko via RT wrote:

Show quoted text
My intention was to define a no-op sub 'color' as a fallback,
if the supplying module could not be loaded or was not available.
So the rest of the app code did not need to change.

Aahh, now I understand.  I can think of some better ways to do that.  Let me know If you care to hear my suggestions.

I don't think there's anything wrong with the Policy then.  A named sub declared inside another sub is always wrong (at least, I can't think of any situations where it would be right).  You might have been thinking of this...

eval 'sub color{}';

...which has a different set of problems and violates a different Policy :)


-Jeff






This service runs on Request Tracker, is sponsored by The Perl Foundation, and maintained by Best Practical Solutions.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.