Bug #72660 for Perl-Critic: Exclude pragmas from Modules::RequireExplicitPackage

I would prefer that the change be a list of exempt modules. I don't like the idea of having a whole class of modules being exempt.

On Wed Nov 23 14:11:43 2011, clonezone wrote: Show quoted text
I think this is not unreasonable, though if it's my fingers at the keyboard the list of exempt modules would probably be empty by default. There actually seem to me to be two issues: what the ticket asks for, and why it was filed. Why the ticket was filed: The motivation for the ticket is that DBIx::Class::Schema::Loader generates code that puts 'use utf8;' before the package declaration. If Unicode package names were supported by Perl, allowing 'use utf8' would be a no brainer. But 'perldoc utf8' says that Unicode package names are not supported, because a package name is, ultimately, a file name, and Unicode file names are not portable. This leaves me believing that it is a stylistic issue, and my feeling is that the title of the book should come before the first chapter, not after. Others may feel differently, but nobody makes anyone use any particular Perl::Critic policy. We have actually been around the "Code generator X generates the code this way so Perl::Critic should accept it" merry-go-round before (with Dist::Zilla and 'BEGIN { require 5.010; }' I believe). And it seems to me that what code generator X does is not relevant in and of itself. It's a question of whether what code generator X does makes sense. What the ticket asks for: The ticket generalizes the problem to asking for any pragma to be accepted before a package statement, justifying this by saying that pragmas are lexical in scope, and therefore span package boundaries. That's fine in theory, but the devil is in the details. The fact is that I know of no way to determine what is a pragma. Adam Kennedy solves this by saying it's a package with an all-lowercase name, and adding all sorts of caveats to the docs. By this definition, 'constant' is a pragma, but manifest constants declared with 'use constant' are scoped to the name space, not lexically. The only other way to recognize a lexically-scoped pragma I can think of is an exhaustive list, which will be out-of-date tomorrow. My conclusion, for what it's worth: What all this says to me is that it would be equally valid to file a ticket against DBIx::Class::Schema::Loader to get them to put the 'use utf8' after the package statement. Since my only reason for this is stylistic that does not mean that we reject the ticket. But it looks to me like a wish list item, with the implementation Elliot suggested.

On Sat Nov 26 11:37:39 2011, clonezone wrote: Show quoted text
Since the discussion here seems to have kind of petered out, I have committed a patch to allow configured modules to be imported before the package statement. The configuration option is 'allow_import_of', though I'm sure someone can think of a better name, and by default no imports are allowed. The patch submitted by the requester, while appreciated, was not used verbatim because of self-compliance concerns. And of course, if further reflection (and contemplation of the submitted code) says that we do not want this, the beauty of a source control system is that it can always be reverted. Oh. It's SVN revision 4098.

On Wed Dec 07 16:46:11 2011, jeff@imaginative-software.com wrote: Show quoted text
Well, I guess the code had better be right then. PPI::Statement::Include->module() returned undef for 'use 5.010;', but in fact it returns. Corrected in SVN revision 4099.

On Thu Dec 08 19:04:20 2011, WYANT wrote: Show quoted text
And the above was found in the throes of adding another configuration option, 'allow_perl_version', to allow the aforementioned 'use 5.010;' before the package statement. This was committed as SVN revision 4100.

On Thu Dec 08 19:27:30 2011, jeff@imaginative-software.com wrote: Show quoted text
Well, to me it made sense for two reasons: First, 'use 5.010;' seems to me different semantically than 'use Foo;'. The first form specifies a version of Perl, the second loads a module. ExtUtils::MakeMaker considers them different, specifying modules via PREREQ_PM, and the minimum Perl with MIN_PERL_VERSION. Module::Build considers them less different, but looks at 'use 5.010;' as being more like requiring version 5.010 of pseudo-module 'perl'. So my read is that the Module::Build approach would be more like [Modules::RequireExplicitPackage] allow_import_of = perl utf8 strict Second, because though they are both represented by a PPI::Statement::Include, if the PPI object represents a 'use 5.010;' the module() method returns false and the version() method returns 5.010, whereas for 'use Foo;' the module() method returns 'Foo' and the version() method returns false. So the two constructions take separate paths through the code anyway. Unless I ignore Adam's take on the situation as well, and just use $elem->schild( 1 ) to retrieve the module/version. Honestly, though, the real reason for not just doing allow_import_of = 5.10 is that it just feels weird and fragile to me. Weird, because that syntax disallows 'use 5.x;' for any 'x' but that explicitly listed. Fragile because it the minimal implementation treats 'use 5.010;', 'use 5.10.0;', 'use v5.10;' as being different. The principal of least surprise says they ought all to be equivalent. Plus, it seems to me if we're really interested in restricting the version of Perl specified by 'use 5.x;', it should be a policy all on its own. Anyhow, that's my take. But I made this a separate commit so that it could be removed if it seemed on discussion to be a bad idea.

On Fri Dec 09 08:10:56 2011, clonezone wrote: Show quoted text
use v5.10 is now equivalent to: BEGIN { require v5.10; no feature; feature->import(':5.10'); } But there is a good possibility that, some time before 5.16, ‘use v5.10’ will not actually load feature.pm to do its work. So considering ‘use v5.10’ equivalent to ‘use feature’ may not be a good idea.

On Sat Dec 10 19:41:15 2011, WYANT wrote: Show quoted text
FYI, I’m not actually using this module. I just wanted to provide clarification as to the feature.pm status, so your decision could be more informed. I.e., I couldn’t care less what the outcome is. :-) Show quoted text

As proposed a couple ticket updates ago, in the absence of dissent, I have removed the allow_perl_version configuration option that was added in commit 4100. The removal was committed as SVN revision 4101.

Le 2011-12-11 01:41:15, WYANT a écrit : Show quoted text
The difference with 'use' statements is the scope on which they apply. Most "pragma" (lower-case module names) apply to lexical scope: strict, warnings, Perl version, utf8, byte, encoding... Some exceptions are well known: base, parent, constant. Most upper-case apply to the package scope as they import symbols into the package. So the rule for allowing a "use" statement before a "package" statement is the scope on which the imported module applies. Any lexical module should be allowed before the 'package' statement, except for stylistic reasons. But there is now easy rule to determine if a module is lexical. The only practical solution is a list of module names. -- Olivier Mengué - http://perlresume.org/DOLMEN