Bug #48219 for Perl-Critic: no warning from Perl::Critic::Utils::is

Sun Jul 26 21:30:54 2009 user42 [...] zip.com.au - Ticket created

Subject:	no warning from Perl::Critic::Utils::is_script() please
Date:	Mon, 27 Jul 2009 11:29:59 +1000
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

In the current cvs I see Perl::Critic::Utils::is_script() prints a warning asking you to use ->is_script. I wish it wouldn't do that, as it makes existing policies like Lax::RequireEndWithTrueConst chatter on endlessly (a warning for every document file processed). It looks like the is_script() code is fine just dispatching to $doc->is_script, I'd hope that a note in the docs about what's now available from PPI would be enough, and keep the function and its current behaviour for compatibility.

Sun Jul 26 22:09:35 2009 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #48219] no warning from Perl::Critic::Utils::is_script() please
Date:	Sun, 26 Jul 2009 21:09:10 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

Kevin Ryde via RT wrote: Show quoted text

> In the current cvs I see Perl::Critic::Utils::is_script() prints a > warning asking you to use ->is_script. > > I wish it wouldn't do that, as it makes existing policies like > Lax::RequireEndWithTrueConst chatter on endlessly (a warning for every > document file processed).

Would you prefer that your policy just stopped working? Show quoted text

> It looks like the is_script() code is fine just dispatching to > $doc->is_script, I'd hope that a note in the docs about what's now > available from PPI would be enough, and keep the function and its > current behaviour for compatibility.

I'm sorry, but I don't expect to keep backwards compatibility on poorly designed interfaces forever (some of them created by me). We will go through a reasonable deprecation cycle, but we will change our interface over time. There have been some recent discussions in the Perl community about what "deprecated" means. For me, "deprecated" means that something WILL BE REMOVED in the future, not languish around for years in a limbo state.

Sun Jul 26 22:09:36 2009 The RT System itself - Status changed from 'new' to 'open'

Tue Jul 28 19:13:08 2009 rjbs [...] cpan.org - Correspondence added

Heck, if somebody told me that this warning started happening (by filing a bug against Lax) I would've been happy to change it immediately. -- rjbs

Wed Jul 29 19:51:14 2009 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #48219] no warning from Perl::Critic::Utils::is_script() please
Date:	Thu, 30 Jul 2009 09:50:15 +1000
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Elliot Shank via RT" <bug-Perl-Critic@rt.cpan.org> writes: Show quoted text

> > Would you prefer that your policy just stopped working?

For non-critical things give authors a time to update, and users a time to install the new, before lots of messages. There's going to be unhappy times when updates are needed post-haste or things have to stop working, but as far as I can tell is_script() is fine and warnings.pm is really quite a blunt instrument to go to immediately. Show quoted text

> what "deprecated" means

Of course it's not a word, but my observation is that in practice it means anything on a continuum from "this never worked properly and sorry but we have to transition away" through to "I gratuitously renamed something others have relied on". The problem is when people are nearer the far end of the spectrum than they care to realize :-)

Wed Jul 29 23:00:01 2009 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #48219] no warning from Perl::Critic::Utils::is_script() please
Date:	Wed, 29 Jul 2009 21:59:39 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

Kevin Ryde via RT wrote: Show quoted text

> "Elliot Shank via RT" <bug-Perl-Critic@rt.cpan.org> writes:

>> what "deprecated" means

> > Of course it's not a word, but my observation is that in practice it > means anything on a continuum from "this never worked properly and sorry > but we have to transition away" through to "I gratuitously renamed > something others have relied on". The problem is when people are nearer > the far end of the spectrum than they care to realize :-)

A name would have to be pretty horrific for me to change it. And I certainly don't plan on pulling what JSON 2.0 did.

Sat Aug 01 20:15:41 2009 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #48219] no warning from Perl::Critic::Utils::is_script() please
Date:	Sun, 02 Aug 2009 10:14:06 +1000
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Elliot Shank via RT" <bug-Perl-Critic@rt.cpan.org> writes: Show quoted text

> > don't plan on pulling what JSON 2.0 did.

I'm not aware of what that is/was. The one that gets up my goat is Gtk where every new way has immediate simultaneous action against the old. As if the existence of the old-but-works somehow offends the new. Or there can't be more than one way to do it. If taken seriously it'd be a sisyphean treadmill of constant update to stay on a shifting notion of approved-way. Umm, err, but at any rate I would like to propose for is_script() something modest like the following. It doesn't mean of course something more can't be done in the future, just no hurry.

Index: Utils.pm =================================================================== --- Utils.pm (revision 3459) +++ Utils.pm (working copy) @@ -845,11 +845,6 @@ sub is_script { my $doc = shift; - warnings::warnif( - 'deprecated', - 'Perl::Critic::Utils::is_script($doc) deprecated, use $doc->is_script() instead.', ## no critic (ValuesAndExpressions::RequireInterpolationOfMetachars) - ); - return $doc->is_script() if blessed($doc) && $doc->isa('Perl::Critic::Document'); return 1 if shebang_line($doc); @@ -1652,6 +1647,9 @@ =item C<is_script( $document )> +This function has been adopted into PPI as C<$document-E<gt>is_script>. +That method is recommended once you're ready to depend on new enough PPI. + Given a L<PPI::Document|PPI::Document>, test if it starts with C</#!.*/>. If so, it is judged to be a script instead of a module. Also, if the filename of the document ends in ".PL" then it is @@ -1661,10 +1659,7 @@ how the document is judged. See C<shebang_line()>. -B<This subroutine is deprecated.> You should use the -L<Perl::Critic::Document/"is_script()"> method instead. - =item C<is_in_void_context( $token )> Given a L<PPI::Token|PPI::Token>, answer whether it appears to be in a

Sat Aug 01 20:28:35 2009 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #48219] no warning from Perl::Critic::Utils::is_script() please
Date:	Sun, 02 Aug 2009 10:27:39 +1000
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

Oh, I've embarrassed myself as between PPI and pc. What I meant of course is the following (even if it could helpfully be a PPI method!).

Index: Utils.pm =================================================================== --- Utils.pm (revision 3459) +++ Utils.pm (working copy) @@ -845,11 +845,6 @@ sub is_script { my $doc = shift; - warnings::warnif( - 'deprecated', - 'Perl::Critic::Utils::is_script($doc) deprecated, use $doc->is_script() instead.', ## no critic (ValuesAndExpressions::RequireInterpolationOfMetachars) - ); - return $doc->is_script() if blessed($doc) && $doc->isa('Perl::Critic::Document'); return 1 if shebang_line($doc); @@ -1652,6 +1647,10 @@ =item C<is_script( $document )> +This function is now a L<Perl::Critic::Document> method +C<$document-E<gt>is_script>. That method is recommended instead of this +function. + Given a L<PPI::Document|PPI::Document>, test if it starts with C</#!.*/>. If so, it is judged to be a script instead of a module. Also, if the filename of the document ends in ".PL" then it is @@ -1661,10 +1660,7 @@ how the document is judged. See C<shebang_line()>. -B<This subroutine is deprecated.> You should use the -L<Perl::Critic::Document/"is_script()"> method instead. - =item C<is_in_void_context( $token )> Given a L<PPI::Token|PPI::Token>, answer whether it appears to be in a

Sat Aug 01 21:36:15 2009 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #48219] no warning from Perl::Critic::Utils::is_script() please
Date:	Sat, 01 Aug 2009 20:35:52 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

Kevin Ryde via RT wrote: Show quoted text

> Oh, I've embarrassed myself as between PPI and pc. What I meant of > course is the following (even if it could helpfully be a PPI method!).

Adam is not likely to support such a thing. (I've tried with stuff I considered a lot more appropriate than this.) But down the road of noting deprecations by merely noting it in the documentation leads the way to making interfaces stagnant for ever. We will evolve slowly, but evolve we will. If all we do is make a note, then no one will change. Thus, the warning. By the way of interface changes, have you been paying attention to the stuff we've been going through to be compatible with the next PPI release? Now there's a significant breakage for you. I highly suggest that you test all your code with PPI 1.204_07 / P::C 1.101_003.

Sat Aug 01 22:02:51 2009 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #48219] no warning from Perl::Critic::Utils::is_script() please
Date:	Sun, 02 Aug 2009 12:01:17 +1000
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Elliot Shank via RT" <bug-Perl-Critic@rt.cpan.org> writes: Show quoted text

> > stagnant for ever.

Well, you say stagnant, I say stable foundation! :) Jokes aside, I believe it's quite important to value usages which were good at the time and are still supportable. It's care with that sort of thing that makes perl and (most of) its libraries attractive -- code from 10 years ago still goes well. (With some irritating exceptions like associative arrays for instance ...) Show quoted text

> paying attention to the stuff we've been going through to be > compatible with the next PPI release?

Only some of it. (I hope the For vs ForLoop can be compatibile-ized say.) Show quoted text

> Now there's a significant breakage for you.

That's bad if it ends up like that. (Maybe there's an epochal cut coming along. I'm not against revamps outright if really called for, though I always wish it can at least be under completely disjoint names, so as not to break the existing, or to make the junction clear to everyone ... if you know what I mean.)

Sun Aug 02 12:30:45 2009 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #48219] no warning from Perl::Critic::Utils::is_script() please
Date:	Sun, 02 Aug 2009 11:30:21 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

Kevin Ryde via RT wrote: Show quoted text

> Jokes aside, I believe it's quite important to value usages which were > good at the time and are still supportable. It's care with that sort of > thing that makes perl and (most of) its libraries attractive -- code > from 10 years ago still goes well.

On the "stability vs. language evolution" debate that Perl has been having of late, I'm towards the language evolution end of the scale. Show quoted text

>> paying attention to the stuff we've been going through to be >> compatible with the next PPI release?

> > Only some of it. (I hope the For vs ForLoop can be compatibile-ized > say.)

He did an ugly sort-of-compatibility hack, which I don't like. Show quoted text

>> Now there's a significant breakage for you.

> > That's bad if it ends up like that. (Maybe there's an epochal cut > coming along. I'm not against revamps outright if really called for, > though I always wish it can at least be under completely disjoint names, > so as not to break the existing, or to make the junction clear to > everyone ... if you know what I mean.)

Unfortunately, he broke up C-style and list iteration loops by making the list iteration conditions be instances of PPI::Structure::List, so, if you want a regular list in the middle of regular code, you now need to check that the list isn't part of the foreach loop.

Tue Aug 04 13:16:53 2009 jeff [...] imaginative-software.com - Correspondence added

Subject:	Re: [rt.cpan.org #48219] no warning from Perl::Critic::Utils::is_script() please
Date:	Tue, 4 Aug 2009 10:16:31 -0700
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Jeffrey Thalhammer <jeff [...] imaginative-software.com>

On Aug 1, 2009, at 7:02 PM, Kevin Ryde via RT wrote: Show quoted text

> Queue: Perl-Critic > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=48219 > > > "Elliot Shank via RT" <bug-Perl-Critic@rt.cpan.org> writes:

>> >> stagnant for ever.

> > Well, you say stagnant, I say stable foundation! :)

I tend to agree with Elliot on this. We're only human, and unfortunately, we make mistakes (especially me). So we need a way to correct those mistakes without having to pay interest on the technical debt forever. So the Perl::Critic project considers "deprecated" to mean "this feature will be removed in the near future." I also agree that changing the documentation just isn't enough. But I do wish we had a more flexible mechanism for emitting warnings. Perhaps if P::C had its own configurable warning mechanism for these sorts of things (For example: "perlcritic -no-deprecation-warnings SomeFile.pm") ?? -Jeff

Tue Aug 04 21:25:27 2009 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #48219] no warning from Perl::Critic::Utils::is_script() please
Date:	Tue, 04 Aug 2009 20:25:05 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

Jeffrey Thalhammer via RT wrote: Show quoted text

> Perhaps if P::C had its own configurable warning mechanism for these > sorts of things

It's called "no warnings 'deprecated';".

Wed Aug 05 13:51:23 2009 wyant [...] cpan.org - Correspondence added

Sorry, it took a while for the penny to drop. I should have put in my $0.02 sooner. In a nutshell, the statement that Perl::Critic::Utils::is_script() simply delegates to Perl::Critic::Document is false. It delegates only if the argument is a Perl::Critic::Document object, and _that_ is the reason for the deprecation. The alternative to deprecation would have been to reject an enhancement request. Deprecation is not done arbitrarily, and as I recall there was some wrangling over this one on the developers' mailing list before this deprecation was done. Recently Perl::Critic was enhanced to allow the user some control over what is a script and what is a module, based on the file name suffix. This is for the benefit of Win32 users, whose scripts tend not to have shebang lines. From both theoretic and pragmatic considerations, this machinery was hung on the Perl::Critic::Document object. But Perl::Critic::Utils::is_script() does not _require_ a Perl::Critic::Document object as input. If given a PPI::Document::File, it will not, CAN NOT, use the enhanced classification machinery. Nor can Perl::Critic::Utils::is_script() usefully manufacture its own Perl::Critic::Document, because it does not have access to the Perl::Critic configuration information it needs to manufacture that object correctly. Certainly if Perl::Critic::Lax is used _only_ in the context of perlcritc (1), Perl::Critic::Utils::is_script() will always be passed a Perl::Critic::Document, and will therefore always do the right thing. But neither the Perl::Critic nor the Perl::Critic::Lax authors can guarantee this. So rather than silently introduce a subtle bug in someone's DarkPAN code, the interface was deprecated. As a postscript, it appears to me that this deprecation is _not_ in released code, though I see no way to remove it without also removing the enhancement that depends on it. Tom Wyant

Tue Aug 11 17:45:30 2009 wyant [...] cpan.org - Correspondence added

Of all the stuff on CPAN that directly depends on Perl::Critic, Perl-Critic-Lax is the only one that calls is_script(). I have not checked anything on CPAN that depends on Perl::Critic indirectly, on the somewhat-dubious principle that if the authors of these modules called Perl::Critic::Utils::is_script() they would make the dependency on Perl::Critic direct. No, I'm not picking on Perl-Critic-Lax, at least not by intent. Just trying to define the scope of the problem. Tom Wyant

Bug #48219 for Perl-Critic: no warning from Perl::Critic::Utils::is_script() please

Preferred bug tracker