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: 64582
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: dschrag [...] oneupweb.com
Cc:
AdminCc:

Bug Information
Severity: Wishlist
Broken in: 1.109
Fixed in: (no value)



Subject: Perl::Critic::PolicyFactory shouldn't warn if profile disables non-installed policy (patch)
Download (untitled) / with headers
text/plain 579b
We have different versions of Perl (and Perl::Critic) installed, but use a common perlcriticrc. We want to disable a few newer policies, but older versions of Perl::Critic don't have those installed and warn because it is mentioned in the user profile. For example, with a .perlcriticrc containing: [-Test::Test] And a file Test.pm containing: package Test; use warnings; use strict; 1; The following warning is unnecessary: dmaestro@dev:~$ perlcritic Test.pm Policy "Perl::Critic::Policy::Test::Test" is not installed. Test.pm source OK dmaestro@dev:~$
Subject: policyfactory.diff
Download policyfactory.diff
text/x-diff 575b
--- Perl/Critic/PolicyFactory.pm.org 2010-11-29 12:06:45.000000000 -0500 +++ Perl/Critic/PolicyFactory.pm 2011-01-07 11:03:25.000000000 -0500 @@ -300,7 +300,8 @@ my %known_policies = hashify( $self->site_policy_names() ); for my $policy_name ( $profile->listed_policies() ) { - if ( not exists $known_policies{$policy_name} ) { + if ( not exists $known_policies{$policy_name} and + not $profile->policy_is_disabled($policy_name) ) { my $message = qq{Policy "$policy_name" is not installed.}; if ( $errors ) {
Subject: Re: [rt.cpan.org #64582] Perl::Critic::PolicyFactory shouldn't warn if profile disables non-installed policy (patch)
Date: Fri, 7 Jan 2011 12:38:13 -0800
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
Download (untitled) / with headers
text/plain 370b
Try setting the --profile-strictness=quiet option. This will suppress that warning. It might also suppress some other non-fatal warnings, but off the top of my head, I'm not sure what those are. Let me know if that doesn't work for you. Happy New Year! Jeffrey Thalhammer Imaginative Software Systems vcard: http://www.imaginative-software.com/contact/jeff.vcf
Download (untitled) / with headers
text/plain 1.1k
On Fri Jan 07 15:38:37 2011, jeff@imaginative-software.com wrote: Show quoted text
> Try setting the --profile-strictness=quiet option. This will suppress > that warning. It might also suppress some other non-fatal > warnings, but off the top of my head, I'm not sure what those are. > Let me know if that doesn't work for you. > > Happy New Year! > > Jeffrey Thalhammer > Imaginative Software Systems > vcard: http://www.imaginative-software.com/contact/jeff.vcf >
Thanks for your response, Jeff. I tried the 'quiet' option, but it also suppresses warnings if attributes are set for a module which is not installed - a case I'd definitely like to know about. I had to think a little more about my request, since turning this warning off might mask a misspelling of a module _intended_ to be disabled; but I think overall I'd rather not be warned. And I'd like our site-wide configuration to be in the .perlcriticrc, not in additional command options. Of course, even if the patch is accepted, I'll have to upgrade all the Perl::Critic installations to that level ;-) Your call (or maybe others will comment if they've run into this?) whether the change is useful enough. DLS
Download (untitled) / with headers
text/plain 860b
On Fri Jan 07 16:04:40 2011, http://dmaestro12.myopenid.com/ wrote: Show quoted text
> On Fri Jan 07 15:38:37 2011, jeff@imaginative-software.com wrote:
> > Try setting the --profile-strictness=quiet option. This will suppress > > that warning. It might also suppress some other non-fatal > > warnings, but off the top of my head, I'm not sure what those are.
> I had to think a little more about my request, since turning this > warning off might mask a misspelling of a module _intended_ to be > disabled; but I think overall I'd rather not be warned. And I'd like our > site-wide configuration to be in the .perlcriticrc, not in additional > command options. >
Oh, I see that can be used easily in the profile itself, so the command-line option isn't needed after all. Though I still don't want it to warn me about _disabling_ options that don't exist. Show quoted text
> > DLS
Subject: Re: [rt.cpan.org #64582] Perl::Critic::PolicyFactory shouldn't warn if profile disables non-installed policy (patch)
Date: Fri, 7 Jan 2011 13:33:37 -0800
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
Download (untitled) / with headers
text/plain 1.6k
On Jan 7, 2011, at 1:04 PM, http://dmaestro12.myopenid.com/ via RT wrote: Show quoted text
> I tried the 'quiet' option, but it also suppresses warnings if > attributes are set for a module which is not installed - a case I'd > definitely like to know about.
True. But with a common configuration, that risk might be pretty low. Each time you edit your config (which is hopefully not too often), you can run your newest version of perlcritic with the --profile-strictness=fatal to verify that it is sane. Everyone else using older versions can just run blindly with --profile-strictness=quiet. Show quoted text
> I had to think a little more about my request, since turning this > warning off might mask a misspelling of a module _intended_ to be > disabled; but I think overall I'd rather not be warned.
Yeah, this could be argued either way. I'll have to think about which scenario is more likely to be the "right" one. Show quoted text
> And I'd like our > site-wide configuration to be in the .perlcriticrc, not in additional > command options.
As with any other command-line option, you can put --profile-strictness in your .perlcriticrc file. Show quoted text
> Of course, even if the patch is accepted, I'll have to upgrade all the > Perl::Critic installations to that level ;-)
Also true. And if everyone has the same Perl::Critic version, then the whole problem goes away. If you've managed to get everyone to use a common config, then hopefully it shouldn't take too much arm wrestling to get everyone to use the same library version. I'll let the other P::C developers comment before making any decisions on the patch. Thanks for sending it. Jeffrey Thalhammer Imaginative Software Systems vcard: http://www.imaginative-software.com/contact/jeff.vcf
Download (untitled) / with headers
text/plain 1.8k
I'm still not completely sure where I come out on this. My knee-jerk is that we should accept the patch. But when I think about it, I tend to believe we should not, at least as submitted. I hate that, especially when someone has gone to the trouble to track down the behavior they would like to change, and come up with a patch for it. It seems a poor reward for their diligence. When I think about it, my model is deleting a file. It can be argued that it should not be an error when you try to delete a file which is not there, because the end state is the same -- no file. But whether you spell delete 'rm', 'del', 'delete', or even 'pip /de', every command-line deletion program I can think of warns on an attempt to delete a non-existent file, at least by default. Most of them also have some way to suppress warnings, and the suppression is not limited to the warning about a non-existent file. If that analogy is any good, then Perl::Critic already works that way. But in that case I'm inclined to see the request as a wish for a way to suppress warnings on disabling non-existent policies, without suppressing anything else. And the question becomes how best to do that. I can think of two ways to go without removing the ability to be warned when a disabled module does not exist: one is another global option (something like --ignore-non-existent-policy, or --non-existent-policy=quiet|warn|fatal); the other is syntax on the module specification in .perlcriticrc (something like [--NonExistent::Policy], or maybe even [-disable -quietly NonExistent::Policy].) The global option has a known implementation path, but if tinkering with the module specification can be made to work it would be more flexible. Of course, no matter what we do, the requester has to upgrade Perl::Critic to get the benefit. But an upgrade would fix his problem even if we did nothing. Make of all this what you will. Tom
Download (untitled) / with headers
text/plain 3.2k
On Sun Jan 09 20:39:58 2011, WYANT wrote: Show quoted text
> I'm still not completely sure where I come out on this. My knee-jerk is > that we should accept the patch. But when I think about it, I tend to > believe we should not, at least as submitted. I hate that, especially > when someone has gone to the trouble to track down the behavior they > would like to change, and come up with a patch for it. It seems a poor > reward for their diligence. >
In my case, I am rewarded sufficiently by your thoughtful responses, so no worries! Show quoted text
> When I think about it, my model is deleting a file. It can be argued > that it should not be an error when you try to delete a file which is > not there, because the end state is the same -- no file. But whether you > spell delete 'rm', 'del', 'delete', or even 'pip /de', every > command-line deletion program I can think of warns on an attempt to > delete a non-existent file, at least by default. Most of them also have > some way to suppress warnings, and the suppression is not limited to the > warning about a non-existent file. > > If that analogy is any good, then Perl::Critic already works that way. > But in that case I'm inclined to see the request as a wish for a way to > suppress warnings on disabling non-existent policies, without > suppressing anything else. And the question becomes how best to do that. > > ... > Of course, no matter what we do, the requester has to upgrade > Perl::Critic to get the benefit. But an upgrade would fix his problem > even if we did nothing. > > Make of all this what you will. > > Tom
I'm more and more persuaded that just suppressing the warning for the case I don't like is not the right thing. It makes the 'strictness' parameter too "potentially surprising". I was definitely surprised by the behavior I saw, but there is adequate provision in the -profile-strictness parameter for most usage. My surprise was really the result of a versioning issue, not a strictness issue, so I think after all that's where it should be addressed (if at all). My humble proposal is this: Add a parameter named '-validate-at-version' which accepts a Perl::Critic version number, or a range between two versions. The actions would be as follows: 1. If the Perl::Critic version in use is equal to the highest version in the validation range, or no parameter is given, validate everything according to the strictness parameter. 2. If a range is given, and the Perl::Critic version falls within this range, but is not the highest, then skip validation errors/warnings that are likely caused by versioning (such as my case where a new Policy is desired to be disabled). Yes, errors as well, for consistency. 3. If the Perl::Critic version is outside the given range, then add a warning/error for ProfileVersionMismatch. Omitting this parameter then behaves exactly as now. Using a single value enforces a specific version of Perl::Critic for the given policy. Using a range value implies that the policy is designed for the latest Perl::Critic version given, but previous versions within the range are known to be suitable as well. Probably should warn in the documentation against using this casually! While I think this is the right answer, I'm not prepared to supply a new patch immediately. Hopefully I can find an opportunity in the next few months if you agree it is worthwhile. Thanks! DLS


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

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