Convention for noting that a dist is intentionally not using strict and warnings

[neilb]

It would be handy if I could include a comment like the following my my code

# CPANTS use strict
# CPANTS use warnings

Or non CPANTS-specific:

# not using strict;
# not using warnings;

This would flag that there's a reason why I'm not using strict and warnings, so don't fail me on that please :-)

See the discussion on questhub for a quest to get all one's dists "CPANTS clean". Both @book and I have modules (Devel::TraceUse and Devel::Dependencies respectively) where we don't want to use strict and warnings because it will affect the behaviour of the modules.

@tobyink suggested the following:

my $cpants = q/
use strict;
use warnings;
#/;

Which I did, but now CPANTS is complaining because my declared dependencies don't match my code :-)

Which means I have to do one of:

• lie about my dependencies, which I don't like
• accept that I fail this quest (never!)
• delete the dist from CPAN (which seems drastic)
• come up with some hack like deleting the relevant entries from %INC and document that the module must be use'd first. Hacky kludge at best.

[charsbar]

Good point. How about adding something like x_cpants_ignore (name to be determined) in META files to tell CPANTS which metrics should be ignored?

[neilb]

yeah, that sounds good.

[book]

The "special comments" approach has the benefit of working with any intentionnaly ignored module, for any checking tool. It has the big drawback of cluttering the code, which becomes even more annoying if the distribution contains several modules.

The META option can be used by other tools than CPANTS, and does not clutter the code. Given that intentionnaly not using some module seems to be a distribution-level choice for an author, going the META route makes more sense.

What about x_no_use with a list of modules? That way, it's useful for more than just CPANTS, and no connection to the CPANTS metrics is needed.

[neilb]

The "special comments" approach has the benefit of working with any intentionnaly ignored module, for any checking tool. It has the big drawback of cluttering the code, which becomes even more annoying if the distribution contains several modules.

The comments approach also has the advantage that it's self documenting, so someone looking at the code thinking "ha, the bozo hasn't even used strict or warnings", gets to see a comment why.

That said, I prefer the metadata approach. There should just be a comment as well :-)

[charsbar]

What about x_no_use with a list of modules? That way, it's useful for more than just CPANTS, and no connection to the CPANTS metrics is needed.

It's ok for me to support something like x_no_use to modify the behavior of some specific metric, but I'm not sure how many people want that level of finer control. Also, if x_no_use helps other tools, it's probably more appropriate to discuss that elsewhere to attract more attention (maybe a topic for QAH 2014?).

I'd rather stick to x_cpants_* (or maybe x_cpants => { ignore => [qw/some metrics/] } or x_cpants => [qw/-some -metrics/]). It's more explicit, and Test::Kwalitee and its friends can reuse the information easily.

Comments to explain why you don't use some modules would always be nice, but comments to trick CPANTS / Module::ExtractUse may not work in the future, especially when someone makes a faster and more reliable module detector than M::EU with something like Compiler::Lexer / Perl::Lexer.

[tobyink]

come up with some hack like deleting the relevant entries from %INC and document that the module must be use'd first.

That can end up producing some nasty warnings:

$ perl -wE'use strict; BEGIN{ delete $INC{q/strict.pm/} }; use strict;'
Subroutine bits redefined at /home/tai/perl5/perlbrew/perls/perl-5.16.1/lib/5.16.1/strict.pm line 23.
Subroutine import redefined at /home/tai/perl5/perlbrew/perls/perl-5.16.1/lib/5.16.1/strict.pm line 42.
Subroutine unimport redefined at /home/tai/perl5/perlbrew/perls/perl-5.16.1/lib/5.16.1/strict.pm line 47.
$

You need to go further and clean all the subs defined by strict.pm out of the symbol table:

$ perl -wE'use strict; BEGIN{ delete $INC{q/strict.pm/}; undef %strict:: }; use strict'
$

This may cause problems if somebody has taken a reference to something that lived in %strict::. Once strict.pm has reloaded, the reference will no longer be pointing to the new things in %strict::.

[book]

I've added the topic for discussion to the Perl QA Hackathon project page.

[karenetheridge]

package Foo;
no strict;
no warnings;

It's pretty clear from that what the author's intention is, and no sugar in comments is necessary.

[dagolden]

I sort of like Karen's self-documenting suggestion, but no warnings won't work on Perls older than 5.6 and it's conceivable (?!?) that someone might want to do that. And asking people to clutter their .pm files with stuff they don't actually need seems like a bad idea.

I prefer @charsbar's suggestion of a top-level x_cpants key in META

x_cpants => { ignore => [ qw/some metrics/ ] }

That keeps the top-level of the META pretty clean and allows for future hints to x_cpants other than 'ignore'.

[karenetheridge]

x_cpants++

[maddingue]

Hmm, maybe I'm arriving a bit late in the discussion, but I remember have a similar one a few years ago, when domm first added the "use strict" metric, and I made the same remark, because I maintain the CPAN version of XSLoader, which deliberately does not use strict. The discussion was to add a commented statement (#use strict;) but I can't remember if the handling was implemented in CPANTS. https://metacpan.org/source/SAPER/XSLoader-0.16/XSLoader.pm

@dagolden warnings::compat provides a warnings module for older Perls

[dagolden]

@maddingue yes, but then I would have to specify a dependency on warnings::compat, simply to be able to say no warnings::compat in order to tell CPANTS not to give me a demerit. This way lies madness. :smile:

[maddingue]

@dagolden call me mad :3 https://metacpan.org/source/SAPER/Sys-Syslog-0.33/Makefile.PL#L60

[dagolden]

@maddingue but you use warnings. I don't think that's crazy at all. What I think is crazy is to make warnings::compat a prereq only to say no warnings.

"You must have this thing installed so I can tell you that I'm not using it!" :-)

[tobyink]

In the interests of bikeshedding, I'd like to propose:

x_cpants_ignore => {
   no_warnings => "I'm not using warnings because I want to support Perl 5.5 and 5.4.",
   no_strict   => "I'm not using strict because I'm a bloody fool",
},

This way people don't just get a free pass to ignore a metric - they actually need to provide an excuse. The excuses for ignored metrics could be displayed as part of the distribution overview on the CPANTS website.

[karenetheridge]

On Sun, Mar 16, 2014 at 07:16:07AM -0700, Toby Inkster wrote:

This way people don't just get a free pass to ignore a metric - they actually need to provide an excuse.

+1

[dagolden]

I have no objection to providing an excuse, but I still want only a single x_cpants top-level key:

x_cpants => {
    ignore => {
       no_warnings => "I'm not using warnings because I want to support Perl 5.5 and 5.4.",
       no_strict   => "I'm not using strict because I'm a bloody fool",
    },
},

That way, any future CPANTS hints can go in the same place.

[karenetheridge]

x_cpants or x_kwalitee? I admit to not always been clear on the distinction :)

[neilb]

+1 to Toby for having to give excuse +1 to David for single top-level

I think of it as CPANTS is the system that produces the kwalitee measure, hence x_cpants is a hint to the system that measures kwalitee.

[rwfranks]

Just a thought. Why introduce new constructs when Perl already has exactly what is needed?

package Dog::Food;

no warnings;             # acceptable Perl syntax
...

[dagolden]

@rwfranks because some people might want to stay back-compat before 5.6. Read up in the thread again for details.

[rwfranks]

[apologies: my mouse battery just died and jumped on the comment button before I'd finished]

CPANTS already knows (or has complained about) minimum perl version, so can avoid penalising pre-5.6 packages.

[neilb]

@rwfranks: there are a number of modules which tell you what modules were used in your code. For example Devel::Dependencies (which I now maintain), Devel::TraceUse and others.

For these modules we just won't want warnings or strict loaded, because you can't tell the difference between modules loaded by Devel::Dependencies and modules loaded by the user's code. Putting in

no warnings;

will still report that the warnings module was loaded.

There are modules that try to be smarter about handling this, but I'm not maintaining any of those :-)

[rwfranks]

I was attempting to breathe life back into Karen E.'s clean and tidy suggestion, by answering David G.'s backward compatibility objection.

A distro with a properly declared ancient perl version in its meta data should be scored as if it had "use warnings" declared in each of its components. "no warnings" and "use warnings" should be treated as equivalent for the purposes of the test. This expresses the idea that the author has some settled opinion about warnings (5.6+), or is prevented from making this explicit by an overriding compatibility requirement (pre-5.6).

If the parser does not have easy access to the meta data, an alternative (somewhat clumsy) mechanism would be to declare the version in the package:

package Camel::Dung;
use strict;
use 5.004_05;
#no warnings;
...

If you are troubled by the idea that authors could fiddle the score by unsubstantiated claims in the meta data, syntax could be checked using the appropriate perl rev with appropriate penalty point tariff applied.

[dagolden]

That's an interesting idea. Whether from metadata or from code, declaring an old perl version as a dependency then scores warnings differently. Even use 5; would be enough if someone doesn't want to have an explicit opinion about how far back to support.

That doesn't help with strict, but then I'm fine with the idea that "no strict" is used to be explicit.

package Grotty::Guts;
use 5.004;
no strict;

That would work for me and avoids junking up META.

[charsbar]

As of this writing...

• CPANTS analyzer does have access to META data, and knows if use 5.xxx exists or not in each file.
• Module::ExtractUse (which CPANTS uses internally) doesn't recognize no stuff. I suppose this is fixable, but I doubt if it's worth trouble and cost.
• use_warnings is an extra metric, and it doesn't affect the game score.

I don't think it a good idea to modify the behavior of use_warnings (or anything) by the declaration of an ancient perl version, because that makes it harder for visitors to know if the module/distribution really warns when necessary or not. Keep it simple and self-explanatory.

I'm also supposing x_cpants (or whatever) is just to ignore some metrics when calculating the final (game) score, not to tweak the result of analysis itself. All the fails, all the errors should be recorded regardless of x_cpants, so that we can investigate them later, or take some statistics.

[rwfranks]

The 'use warnings' test is meaningless when applied to a distro declaring a dependency on pre-5.6 perl in its meta data. Whether you record a "pass" or "N/A" in the database, and where in the code you do it, are matters for you decide. It is gross miscarriage of justice to penalise code merely for still being alive after 15 or 20 years.

'use warnings' and 'no warnings' are equally good indications that the author has engaged with the topic sufficiently to decide one way or the other. There is no need for CPANTS to record whether the negative form of pragma was used, or to do anything different with it. Knobbling the front-end of the analyser to turn 'no warnings' into 'use warnings' on the fly would solve the problem admirably. Deal with 'no strict' in a similar way. There is clearly some value in doing this, otherwise nobody would be discussing it here.

[karenetheridge]

On Sat, Mar 22, 2014 at 07:20:21AM -0700, rwfranks wrote:

The 'use warnings' test is meaningless when applied to a distro declaring a dependency on pre-5.6 perl in its meta data. Whether you record a "pass" or "N/A" in the database, and where in the code you do it, are matters for you decide. It is gross miscarriage of justice to penalise code merely for still being alive after 15 or 20 years.

"penalise" is a bit harsh since this is just a heuristic. No one's being sent to jail or anything. I'm not going to cry into my cereal because some of my dists are failing other metrics (for instance prereq_matches_use, which sometimes is impossible to pass).

Here's a suggestion for a way forward: we could introduce a third state for all metrics: "pass", "fail", or "mu" (insufficient information to provide an assessment). For distributions that explicitly state a perl version prereq, and it is earlier than 5.6, give the 'use warnings' metric a "mu". If no perl prereq is stated, they fail both that metric and the "use warnings" metric (if the warnings pragma is not used). Likewise, a literal "no warnings" will generate a mu. This score will not be calculated in the overall total at all - e.g. the score will be calculated out of 130 rather than 133.

[charsbar]

As for use_warnings, there is a way to pass even for pre-5.6 perls (use warnings::compat). So, using it or not is just a matter of choice. (FYI, the current analyzer counts strict/warnings equivalents as well, though there was a bug in MCA and it wasn't counted correctly).

no warnings/strict (or their equivalents) are not always used to show a module is totally warnings/strict-free. I don't want to turn no Moose into mu :p

That said, introducing a third state is a todo that I couldn't finish during the QAH this year. I thought of missing META.* for META related metrics etc, and also of tentative metrics (such as prereq stuff) that can't be determined until everything is settled, but it might be applicable to use_warnings as well.

[brad-mac]

Being able to exclude files or directories from Kwalitee checking would mean that testing files aren't picked up in error: http://cpants.cpanauthors.org/dist/Dist-Zilla-Plugin-NexusRelease-0.0.1/errors

All 3 of those fails are for a file that's there to be used as a test subject; and one of the fails is that the test subject library isn't in the lib/ directory...

[charsbar]

@brad-mac that's because you haven't excluded anything. See the following: https://metacpan.org/pod/CPAN::Meta::Spec#no_index https://metacpan.org/pod/Dist::Zilla::Plugin::MetaNoIndex https://metacpan.org/source/RJBS/Dist-Zilla-5.031/dist.ini#L14

[brad-mac]

Nice - and that's documented now (assuming other people go through the same set of Google searches I did and end up here) in a way that I find a bit more obvious. There wasn't anything that jumped out at me about indexing in the Test::Kwalitee docs or on http://cpants.cpanauthors.org/kwalitee.

Thanks!

[karenetheridge]

@charsbar I think it's the meta 'provides' field that's significant here, not no_index?

https://metacpan.org/source/ISHIGAKI/Module-CPANTS-Analyse-0.96/lib/Module/CPANTS/Kwalitee/FindModules.pm#L20

(for @brad-mac I use [MetaProvides::Package] to set this data in my distributions.)

[charsbar]

@karenetheridge whichever is ok as long as it excludes stuff in corpus/ directory correctly. use_strict/use_warnings metrics are only applied to public modules.

[brad-mac]

Thanks Karen - much better than my first attempt with [MetaProvides::Class] :-)

@charsbar, I've forked M:C:A and added some documentation to the 'remedy' for 'proper_libs' - hope that's up to scratch.

[charsbar]

@brad-mac, thanks. The source of your fork is an old one, but applied it by hand anyway: https://github.com/cpants/Module-CPANTS-Analyse/commit/b6a525eacff2b5fde9f4085bbb68d93445db4b3f

[karenetheridge]

More usecases for this metadata:

• Crypt-SSLeay to ignore the "inconsistent versions" check (see https://github.com/cpants/Module-CPANTS-SiteKwalitee/issues/4)
• Acme-Pi (also has intentionally inconsistent versions between module and distribution).