On 1/15/2015 11:09 AM, Mark Martinec wrote:
:-) Well I don't want to change the vetting process for a release and
xt/60_perlcritic.t has been used for years on the code base.
Suggestions what we can do to resolve the issue that also passes that
test so we don't have to go down that rabbithole?
There are some other perlcritic warnings about modifying $_ in list
functions
(in sa-update, spamassassin, spamd). Opening a PR would be warranted.
What is a PR? A bugzilla ticket?
Also, running prove -v xt/60_perlcritic.t shows just the one issue and I
committed the change which reverses some fixes and the original return
undef change in Util.pm.
not ok 38 - Test::Perl::Critic for "../blib/lib/Mail/SpamAssassin/Util.pm"
# Failed test 'Test::Perl::Critic for
"../blib/lib/Mail/SpamAssassin/Util.pm"'
# at /usr/local/lib/perl5/site_perl/5.8.6/Test/Perl/Critic.pm line 110.
#
# Perl::Critic found these violations in
"../blib/lib/Mail/SpamAssassin/Util.pm":
# "return" statement with explicit "undef" at line 287, column 5.
If there are other perl critic warnings, they are likely not enabled in
that test.
Looking back, this return undef was added after 3.4.0's release and
appears to have been added during the profiling work on
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6854
Do you feel confident enough about your change that I should modify or
notate the build process notes to ignore that issue for that specific
return undef?
The MS::Util::untaint_var() is still broken...
I'm reverting some recent changes and reverting back to the one undef in
there. Sorry about that. Best of intentions.
Btw, common use of 'return 0' in TxRep to indicate a boolean response
falls into the same category as 'return undef', although
Test::Perl::Critic
does not check for that.
I use no warnings 'uninitialized', 'once'; for a lot of my day to day
work so I can see that but my key concern is passing the tests required
of a release without trying to change those tests right now.
For a decade or so, we've been able to make releases that pass that
60_perlcritic test and don't cause real-world issues. I'm confident
that we can do so at least in this case. I want to get the release out
by Jan 30. Then I think you are correct we need to look at if this
makes sense longer-term.
Regards,
KAM
