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: 69234
Status: open
Priority: 0/
Queue: Perl-Critic
People
Owner: Nobody in particular
Requestors: xenoterracide [...] gmail.com
Cc:
AdminCc:
Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)
Attachments
critictest.tar.bz2

History Show all quoted text
  Fri Jul 01 15:50:55 2011 xenoterracide [...] gmail.com - Ticket created
Subject: new 5.014 package { }; syntax breaks many tests
Date: Fri, 1 Jul 2011 14:50:44 -0500
To: bugs-perl-critic [...] rt.cpan.org
From: Caleb Cushing <xenoterracide [...] gmail.com>
attached is a repo where the tests demonstrate the issues # Failed test 'Test::Perl::Critic for "blib/lib/critictest.pm"' # at /home/ccushing/perl5/perlbrew/perls/perl-5.14.1/lib/site_perl/5.14.1/Test/Perl/Critic.pm line 110. # # Perl::Critic found these violations in "blib/lib/critictest.pm": # Module does not end with "1;" at line 1, column 1. Must end with a recognizable true value. (Severity: 4) # Code before strictures are enabled at line 4, column 2. See page 429 of PBP. (Severity: 5) # Code before warnings are enabled at line 4, column 2. See page 431 of PBP. (Severity: 4) # Looks like you failed 1 test of 1. t/author-critic.t ............ Dubious, test returned 1 (wstat 256, 0x100) Failed 1/1 subtests this should work. package critictest { use 5.014; use warnings; BEGIN { # VERSION } 1; } -- Caleb Cushing http://xenoterracide.com
Download critictest.tar.bz2
application/x-bzip2 57.2k

Message body not shown because it is not plain text.

  Fri Jul 01 21:38:40 2011 wyant [...] cpan.org - Correspondence added
On Fri Jul 01 15:50:55 2011, XENO wrote: <snip /> Show quoted text
> > this should work. > > package critictest { > use 5.014; > use warnings; > BEGIN { > # VERSION > } > > 1; > } > > >
It appears to me that the policy works as it should. The policy is not about the last thing in a name space, but about the last thing in a module. The 5.14 syntax is pretty much equivalent to { package Foo; 1; } which is also flagged by this policy. What it wants, in 5.14-speak, is package Foo { }; 1; Putting the '1;' inside the brackets makes things more complex, not only for the policy to figure out, but for the reader. Our toy examples are no problem, but what if the block (or whatever) was several hundred lines long? Maybe one of the other maintainers sees it differently, but I am not, at this point, persuaded to make the modification.
  Fri Jul 01 21:38:41 2011 The RT System itself - Status changed from 'new' to 'open'
  Fri Jul 01 22:11:30 2011 xenoterracide [...] gmail.com - Correspondence added
Subject: Re: [rt.cpan.org #69234] new 5.014 package { }; syntax breaks many tests
Date: Fri, 1 Jul 2011 21:11:19 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Caleb Cushing <xenoterracide [...] gmail.com>
On Fri, Jul 1, 2011 at 8:38 PM, Tom Wyant via RT <bug-Perl-Critic@rt.cpan.org> wrote: Show quoted text
> Putting the '1;' inside the brackets makes things more complex, not only > for the policy to figure out, but for the reader. Our toy examples are > no problem, but what if the block (or whatever) was several hundred > lines long? > > Maybe one of the other maintainers sees it differently, but I am not, at > this point, persuaded to make the modification.
wait the return 1 is supposed to go outside the } how's that make much sense? what if you have multiple packages in a file... and why would it be { package ... 1 } and nowt package { 1} . package { } 1 would be totally in a different lexical scope. to be honest I'm not 100% sure what it's supposed to be... but even if putting it on the outside is correct... there are still failing tests I tried putting the 1 on the outside and I still get failing tests. use strict and use warnings should be failed as code coming before. package critictest { use 5.014; use warnings; BEGIN { # VERSION } }; 1; # Failed test 'Test::Perl::Critic for "blib/lib/critictest.pm"' # at /home/xenoterracide/perl5/perlbrew/perls/perl-5.14.1/lib/site_perl/5.14.1/Test/Perl/Critic.pm line 110. # # Perl::Critic found these violations in "blib/lib/critictest.pm": # Code before strictures are enabled at line 4, column 2. See page 429 of PBP. (Severity: 5) # Code before warnings are enabled at line 4, column 2. See page 431 of PBP. (Severity: 4) # Looks like you failed 1 test of 1. -- Caleb Cushing http://xenoterracide.com
  Fri Jul 01 23:48:26 2011 jeff [...] imaginative-software.com - Correspondence added
Subject: Re: [rt.cpan.org #69234] new 5.014 package { }; syntax breaks many tests
Date: Fri, 1 Jul 2011 20:48:15 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
On Jul 1, 2011, at 7:11 PM, Caleb Cushing via RT wrote: Show quoted text
> to be honest I'm not 100% sure what it's supposed to be... but even if > putting it on the outside is correct... there are still failing tests
I think Tom is right. The 1; goes at the end of the file, not (necessarily) the end of the package. Show quoted text
> I tried putting the 1 on the outside and I still get failing tests. > use strict and use warnings should be failed as code coming before.
I suspect PPI doesn't parse the new package { ... } syntax correctly. But I haven't verified this. Tom? -Jeff
  Sat Jul 02 12:32:01 2011 xenoterracide [...] gmail.com - Correspondence added
Subject: Re: [rt.cpan.org #69234] new 5.014 package { }; syntax breaks many tests
Date: Sat, 2 Jul 2011 11:31:50 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Caleb Cushing <xenoterracide [...] gmail.com>
On Fri, Jul 1, 2011 at 10:48 PM, Jeffrey Thalhammer via RT <bug-Perl-Critic@rt.cpan.org> wrote: Show quoted text
> think Tom is right.  The 1; goes at the end of the file, not (necessarily) the end of the package.
yeah you guys are right, it's to return success of loading the file. However I don't see then that { package 1; } makes sense... it would seem that { package } 1; would be correct. or { package } {package} 1; irregardless that's my bad... but strict/warnings still seem to not be detected. Unless of course we're considering that they're lexical in scope and then not enabled for the whole file? -- Caleb Cushing http://xenoterracide.com
  Sat Jul 02 17:05:31 2011 wyant [...] cpan.org - Correspondence added
On Sat Jul 02 12:32:01 2011, XENO wrote: Show quoted text
> On Fri, Jul 1, 2011 at 10:48 PM, Jeffrey Thalhammer via RT > <bug-Perl-Critic@rt.cpan.org> wrote:
> > think Tom is right.  The 1; goes at the end of the file, not
> (necessarily) the end of the package. > > yeah you guys are right, it's to return success of loading the file. > However I don't see then that { package 1; } makes sense... it would > seem that { package } 1; would be correct. or { package } {package} > 1; irregardless that's my bad... but strict/warnings still seem to not > be detected. Unless of course we're considering that they're lexical > in scope and then not enabled for the whole file? >
You've got it. The strict/warnings are lexically scoped. If you want them to cover the whole file they must come first, and that is what the policies ask for. Don't read too much into the 'package Foo {...}' syntax; Perl has not suddenly become a language like Java where pretty much everything has to be inside a class declaration. Jeff - it actually looks to me like PPI handles this. I read the following dump to say that the block is part of the package statement. If I weren't already impressed by Adam, I would be now. $ ppidump 'package Foo { bar() }; 1;' PPI::Document PPI::Statement::Package [ 1, 1, 1 ] PPI::Token::Word 'package' [ 1, 9, 9 ] PPI::Token::Word 'Foo' PPI::Structure::Block { ... } PPI::Statement [ 1, 15, 15 ] PPI::Token::Word 'bar' PPI::Structure::List ( ... ) [ 1, 22, 22 ] PPI::Token::Structure ';' PPI::Statement [ 1, 24, 24 ] PPI::Token::Number '1' [ 1, 25, 25 ] PPI::Token::Structure ';'