Skip to content

Add expect_stderr = true to Perl::Critic example#32

Closed
oalders wants to merge 1 commit intohouseabsolute:masterfrom
oalders:perlcritic-example
Closed

Add expect_stderr = true to Perl::Critic example#32
oalders wants to merge 1 commit intohouseabsolute:masterfrom
oalders:perlcritic-example

Conversation

@oalders
Copy link
Contributor

@oalders oalders commented Sep 20, 2022

Perl::Critic can emit a warning without claiming that there has been a
violation:

https://metacpan.org/release/PETDANCE/Perl-Critic-StricterSubs-0.06/source/lib/Perl/Critic/Policy/Subroutines/ProhibitCallsToUnexportedSubs.pm#L198

This one bit me because I copy/pasted the example from this repo and
just one file started failing for seemingly mysterious reasons.

Perl::Critic can emit a warning without claiming that there has been a
violation:

https://metacpan.org/release/PETDANCE/Perl-Critic-StricterSubs-0.06/source/lib/Perl/Critic/Policy/Subroutines/ProhibitCallsToUnexportedSubs.pm#L198

This one bit me because I copy/pasted the example from this repo and
just one file started failing for seemingly mysterious reasons.
@oalders
Copy link
Contributor Author

oalders commented Sep 20, 2022

Please merge as you see fit. It's not October yet.

@autarch
Copy link
Member

autarch commented Sep 24, 2022

Is that warning not something you want to fail on? It's not clear to me from the code whether it represents a failure or something you can ignore.

@oalders
Copy link
Contributor Author

oalders commented Sep 30, 2022

use strict;
use warnings;

use Git::Sub qw( config );

my @conf = git::config('--list');
perlcritic --noprofile --single-policy Subroutines::ProhibitCallsToUnexportedSubs git-sub.pl
Subroutines::ProhibitCallsToUnexportedSubs: Cannot find source file "git.pm"
Subroutine "config" not exported by "git" at line 6, column 12.  Violates encapsulation.  (Severity: 4)

I think that warning is a shortcoming of the logic in that particular policy. See https://metacpan.org/pod/Perl::Critic::Policy::Subroutines::ProhibitCallsToUnexportedSubs#LIMITATIONS

So, the violation is correct, but the warning should not be what causes the failure.

This Policy does not properly deal with the only pragma or modules that don't use Exporter for their export mechanism, such as CGI. In the near future, we might fix this by allowing you configure the policy with a list of packages that are exempt from this policy.

Maybe @petdance has an opinion on this? I will open a related ticket on the Perl::Critic repo.

@autarch
Copy link
Member

autarch commented Oct 1, 2022

I think a better fix might be to add a new ignore_stderr config param that lets you provide a list of regexes. If any regex matches then the stderr output is ignored. The current expect_stderr is a pretty blunt instrument. It's good for commands that always print to stderr (which is _very annoying) but not for this particular case.

@oalders
Copy link
Contributor Author

oalders commented Oct 1, 2022

Yes, that sounds a lot more flexible.

@autarch
Copy link
Member

autarch commented Oct 2, 2022

I just release v0.3.0 with the new ignore_stderr param. I think we could update this example config file to show a commented-out version of that param for this particular warning, along with a comment explaining when you might want this.

@autarch autarch closed this Oct 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants