Patches pushed, leaving bug open to catch more 

Now passing for 130 files

Passing latest patch through QA (also consider this my sign off)

All pushed, leaving open for me

The 2nd patch does not work well I think (it also does not apply, but it's easily fixed)

in showmarc.pl, before your patch, my biblio with diacritic look like :
200 1  _aMassacre (The) éô ï
(éô ï is what i'm expecting)
After the patch, it looks like :
200 1  _aMassacre (The) éô ï
which is the sign of a double encoded biblio record.

Marking "failed QA" for now. The 3rd patch, that updates perlcritic.tt does not apply after that (i've not tested further, I suspect they will conflict because catalogue has not been added

Once the showmarc issue is solved, i'll test & push those usefull code cleaning patches as fast as possible

Comment on attachment 6144 [details] [review]
Bug 6679 : Tidying changed parts of file

this patch has been pushed

Comment on attachment 6143 [details] [review]
Bug 6679 : Don't declare variables in conditionals

this patch has been pushed

Comment on attachment 6142 [details] [review]
Bug 6679 : Not using strict .. bad script, no cookie for you!

this patch has been pushed

Comment on attachment 6141 [details] [review]
Bug 6679 : Don't declare variables in conditionals Don't change $_ in a list context http://search.cpan.org/~elliotjs/Perl-Critic-1.116/lib/Perl/Critic/Policy/ControlStructures/ProhibitMutatingListFunctions.pm

this patch has been pushed

Comment on attachment 6140 [details] [review]
Bug 6679 : Declare variables outside of conditionals

This patch has been pushed

Comment on attachment 6121 [details] [review]
Bug 6679: Fixing admin/aqplain :encoding(UTF-8) gives strict validation

this patch has been pushed

Comment on attachment 6122 [details] [review]
Bug 6679: catalogue/ now passing perl critic tests

patch pushed

Comment on attachment 6123 [details] [review]
Bug 6679 : cataloguing/ now passing critic tests

patch pushed.

trying to prove cataloguing show me some errors in plugins though:

prove t/00-testcritic.t 
t/00-testcritic.t .. 196/246 
#   Failed test 'Test::Perl::Critic for "cataloguing/value_builder/stocknumber.pl"'
#   at /usr/share/perl5/Test/Perl/Critic.pm line 110.
# 
# Perl::Critic found these violations in "cataloguing/value_builder/stocknumber.pl":
# Code before strictures are enabled at line 30, column 1.  See page 429 of PBP.  (Severity: 5)
t/00-testcritic.t .. 239/246 
#   Failed test 'Test::Perl::Critic for "cataloguing/value_builder/unimarc_field_210c.pl"'
#   at /usr/share/perl5/Test/Perl/Critic.pm line 110.
# 
# Perl::Critic found these violations in "cataloguing/value_builder/unimarc_field_210c.pl":
# Code before strictures are enabled at line 40, column 1.  See page 429 of PBP.  (Severity: 5)
# Variable declared in conditional statement at line 113, column 9.  Declare variables outside of the condition.  (Severity: 5)
t/00-testcritic.t .. 243/246 
#   Failed test 'Test::Perl::Critic for "cataloguing/value_builder/unimarc_field_4XX.pl"'
#   at /usr/share/perl5/Test/Perl/Critic.pm line 110.
# 
# Perl::Critic found these violations in "cataloguing/value_builder/unimarc_field_4XX.pl":
# Variable declared in conditional statement at line 272, column 9.  Declare variables outside of the condition.  (Severity: 5)
# Variable declared in conditional statement at line 286, column 9.  Declare variables outside of the condition.  (Severity: 5)
# Variable declared in conditional statement at line 292, column 9.  Declare variables outside of the condition.  (Severity: 5)
# Variable declared in conditional statement at line 377, column 13.  Declare variables outside of the condition.  (Severity: 5)
t/00-testcritic.t .. 245/246 # Looks like you failed 3 tests of 246.


I think it's worth activating the test (it's better to have a test showing something is wrong than doing no test and thinking things are OK ;-) )

Comment on attachment 6139 [details] [review]
Bug 6679 : Adding license statement and stopping declaring variables in a conditional

patch pushed

Comment on attachment 6663 [details] [review]
Bug 6679 : Allowing eval require

patch pushed

Comment on attachment 6270 [details] [review]
Reverting lines which break label-print.pl

patch already applied (bug 7207)

Comment on attachment 6271 [details] [review]
Fixing critic complaint: Don't change $_ in a list context

patch already applied (bug 7207)

The latest round of patches for this bug do not apply cleanly to 3.6.x. Please attach corrected patches to this bug report.

Which patches chris_n .. they have been marked obsolete and I have lost track of them,

If you list the ones you need ill rebase them.

(In reply to comment #41)
> Which patches chris_n .. they have been marked obsolete and I have lost track
> of them,
> 
> If you list the ones you need ill rebase them.

Here are the commit ids:

4a14e7dbe69db0517d1d333a644c4abeb0a42cb8
a047dea5e46338d3d664935451764fffe56e6d9e
dc6e1031f8c2aa36e88ca39a70c5b7d9fd63065d

Comment on attachment 6675 [details] [review]
Bug 6679: fix missing use strict

patch pushed

Chris_n, please apply those patches, then back to "priority 1" and no patch status, until more tests are provided

These patches were applied to 3.6.x last week.

trivial patch

Comment on attachment 8542 [details] [review]
Bug 6679 : Fixing perlcritic violation

this patch has been pushed today

Comment on attachment 8897 [details] [review]
Bug 6679 : Fixing perlcritic violation in authorities/ysearch.pl

I signed off the same patch (from Colin) today. (Bug 7816)

Both signed off patches faithfully replace functionality with Perl::Critic compliant code.  Marking as Passed QA.

Comment on attachment 8950 [details] [review]
Bug 6679 :[SIGNED-OFF] members dir now passes perlcritic tests

This patch has been pushed

Comment on attachment 8951 [details] [review]
Bug 6679 : [SIGNED-OFF] Fixing some perlcritic violations in the opac

This patch has been pushed

A few scripts still not perlcritic compliant in opac/ directory:
opac/changelanguage.pl: Code before strictures are enabled at line 24, column 1.  See page 429 of PBP.  (Severity: 5)
opac/ilsdi.pl: Stricture disabled at line 217, column 17.  See page 429 of PBP.  (Severity: 5)
opac/opac-detail.pl: Variable declared in conditional statement at line 139, column 9.  Declare variables outside of the condition.  (Severity: 5)
opac/opac-detail.pl: Variable declared in conditional statement at line 140, column 9.  Declare variables outside of the condition.  (Severity: 5)
opac/opac-detail.pl: Variable declared in conditional statement at line 416, column 1.  Declare variables outside of the condition.  (Severity: 5)
opac/opac-reserve.pl: Subroutine prototypes used at line 51, column 1.  See page 194 of PBP.  (Severity: 5)

Fixing 26 perlcritic violations in C4/Tags.pm
Apply? [yn] y

Applying: Fixing 26 perlcritic violations in C4/Tags.pm
/home/jonathan/workspace/versions/koha_coderun/.git/rebase-apply/patch:32: trailing whitespace.
sub approval_counts { 
error: patch failed: opac/opac-tags.pl:43
error: opac/opac-tags.pl: patch does not apply
Patch failed at 0001 Fixing 26 perlcritic violations in C4/Tags.pm

All patches rebased on latest master. Signoff needed. :)

FYI - Still additional PBP violations in TmplTokenizer.pm and C4/Barcodes.pm

Comment on attachment 10075 [details] [review]
[SIGNED-OFF] Bug 6679: Fixing 13 perlcritic violations in C4/Barcodes.pm

OK, this patch fixes error, but there are still some:
"return" statement with explicit "undef" at line 57, column 2.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 131, column 3.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 147, column 22.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 152, column 22.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 157, column 22.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 193, column 3.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 197, column 3.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 228, column 2.  See page 199 of PBP.  (Severity: 5)

Comment on attachment 10076 [details] [review]
[SIGNED-OFF] Bug 6679: Fixing 24 perlcritic violations in misc/translator/TmplTokenizer.pm

Patch pushed, still one perlcritic problem in this file:
"return" statement with explicit "undef" at line 319, column 17.  See page 199 of PBP.  (Severity: 5)

Comment on attachment 10077 [details] [review]
[SIGNED OFF] Bug 6679: Fixing 5 perlcritic violations in opac/*

patch pushed,OPAC almost perlcritic compliant, just one error to solve:
opac/ilsdi.pl: Stricture disabled at line 217, column 17.  See page 429 of PBP.  (Severity: 5)

Comment on attachment 10078 [details] [review]
[SIGNED OFF] Bug 6679: Fixing 23 perlcritic violations in C4/Tags.pm

I can't apply this one:
Applying: Bug 6679: Fixing 23 perlcritic violations in C4/Tags.pm
fatal: sha1 information is lacking or useless (C4/Tags.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 6679: Fixing 23 perlcritic violations in C4/Tags.pm
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Comment on attachment 10078 [details] [review]
[SIGNED OFF] Bug 6679: Fixing 23 perlcritic violations in C4/Tags.pm

This patch does not apply, replaced by another one

Comment on attachment 10079 [details] [review]
[SIGNED OFF] Bug 6679: Fixing 26 perlcritic violations in C4/Tags.pm

this patch has been pushed

(In reply to comment #85)
> Comment on attachment 10079 [details] [review]
> [SIGNED OFF] Bug 6679: Fixing 26 perlcritic violations in C4/Tags.pm
> 
> this patch has been pushed

Still a lot of things to update in C4/Tags.pm:
"return" statement with explicit "undef" at line 120, column 25.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 126, column 27.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 148, column 10.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 154, column 10.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 160, column 10.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 166, column 10.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 362, column 22.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 373, column 22.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 387, column 23.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 409, column 23.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 422, column 23.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 429, column 23.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 438, column 22.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 463, column 23.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 464, column 26.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 473, column 30.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 474, column 30.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 487, column 10.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 508, column 47.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 560, column 30.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 561, column 30.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 565, column 13.  See page 199 of PBP.  (Severity: 5)
"return" statement with explicit "undef" at line 574, column 3.  See page 199 of PBP.  (Severity: 5)

(In reply to comment #88)
> Created attachment 10485 [details] [review]
> Bug 6679 - fix 19 perlcritic violations from C4::Letters.pm


FAILED QA for this patch, I think the following lines don't have to be modified:
-    return unless exists $params->{'letter'};
-    return unless exists $params->{'borrowernumber'};
-    return unless exists $params->{'message_transport_type'};
+    return exists $params->{'letter'};
+    return exists $params->{'borrowernumber'};
+    return exists $params->{'message_transport_type'};

It's unless, not undef :-)

Comment on attachment 10517 [details] [review]
Bug 6679 - [FAILED QA]fix 3 perlcritic violations in C4/ClassSortRoutine.pm

Failed QA :
"Bad name after :: at C4/ClassSortRoutine.pm line 55."

Comment on attachment 10517 [details] [review]
Bug 6679 - [FAILED QA]fix 3 perlcritic violations in C4/ClassSortRoutine.pm

Failed QA:
Bad name after :: at C4/ClassSortRoutine.pm line 55.

All seems good except 2 patchs: C4/Letters.pm and C4/ClassSortRoutine.pm

Switch to Assigned

(In reply to comment #158)
> Created attachment 12224 [details] [review]
> Proposed patch
> 
> This patch fixes two instances that cause Jemkins builds to fail

Why don't you just remove the "undef" word ?
I think
if ($@) {
    return;
}
is less readable as
return if $@;

Don't you think ?
I can provide a new patch but I don't want to go against what you want.

Comment on attachment 12363 [details] [review]
Proposed patch to calm Jenkins

This patch has been pushed

patches are signed off, going to this status to have patches in QA team radar !

Comment on attachment 10485 [details] [review]
[FAILED-QA]Bug 6679 - fix 19 perlcritic violations from C4::Letters.pm

This patch is now useless, the same goal has been achieved by a follow-up on bug 8063

Comment on attachment 10494 [details] [review]
Bug 6679 - [SIGNED-OFF]fix 12 perlcritic violations in C4/External/BakerTaylor.pm

This patch has been pushed to master

Comment on attachment 10495 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 12 perlcritic violations in C4/Auth_with_ldap.pm

This patch has been pushed to master

Comment on attachment 10496 [details] [review]
Bug 6679 -[SIGNED-OFF] fix 12 perlcritic violations in C4/Context.pm

This patch has been pushed to master

Comment on attachment 10497 [details] [review]
Bug 6679 - [SIGNED-OFF]fixing 23 '"return" statement with explicit "undef"' violations in C4/Tags.pm

This patch has been pushed to master

Comment on attachment 10517 [details] [review]
Bug 6679 - [FAILED QA]fix 3 perlcritic violations in C4/ClassSortRoutine.pm

This patch has NOT been pushed.
After applying it, you get a Perl error

Comment on attachment 10935 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 15 perlcritic violations in C4/ImportExportFramework.pm

This patch has been pushed to master

Comment on attachment 10936 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 11 perlcritic violations in C4/Dates.pm

This patch has NOT been pushed. After applying it you get a perl error

Comment on attachment 10938 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 9 perlcritic violations in C4/TmplTokenType.pm

This patch has been pushed to master

Comment on attachment 10940 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 9 perlcritic violations in C4/Koha.pm

This patch has been pushed to master

Comment on attachment 10946 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 8 perlcritic violations in C4/Circulation.pm

This patch has been pushed to master

Comment on attachment 10947 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 8 perlcritic violations in C4/Barcodes.pm

This patch has been pushed to master

Comment on attachment 10948 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 7 perlcritic violations in C4/Biblio.pm

This patch has been pushed to master

Comment on attachment 10950 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 6 perlcritic violations in C4/Barcodes/hbyymmincr.pm

This patch has been pushed to master

Comment on attachment 10952 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 5 perlcritic violations in C4/Serials.pm

This patch has been pushed to master

Comment on attachment 10953 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 4 perlcritic violations in C4/ShelfBrowser.pm

This patch has been pushed to master

Comment on attachment 10955 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 4 perlcritic violations in C4/Branch.pm

This patch has been pushed to master

Comment on attachment 10957 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 3 perlcritic violations in C4/SQLHelper.pm

This patch has been pushed to master

Comment on attachment 10959 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 3 perlcritic violations in C4/Output.pm

This patch has been pushed to master

Comment on attachment 10960 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 3 perlcritic violations in C4/Message.pm

This patch has been pushed to master

Comment on attachment 10961 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 3 perlcritic violations in C4/Items.pm

This patch has been pushed to master

Comment on attachment 10963 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 2 perlcritic violations in C4/Utils.pm

This patch has been pushed to master

Comment on attachment 10964 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 2 perlcritic violations in C4/ItemCirculationAlertPreference.pm

This patch has been pushed to master

Comment on attachment 10965 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 2 perlcritic violations in C4/Installer/PerlModules.pm

This patch has been pushed to master

Comment on attachment 10966 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 2 perlcritic violations in C4/BackgroundJob.pm

This patch has been pushed to master

Comment on attachment 10981 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 3 perlcritic violations in C4/Auth.pm, and enabled warnings

This patch has been pushed to master

Comment on attachment 10980 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 2 perlcritic violations in C4/Print.pm

This patch has been pushed to master

Comment on attachment 10979 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 4 perlcritic violations in C4/Creators/PDF.pm

This patch has been pushed to master

Comment on attachment 10976 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 4 perlcritic violations in C4/Installer.pm

This patch has been pushed to master

Attachment that are still here either:
 * does not apply anymore
 * badly break Koha (resulting in a Perl error)

Comment on attachment 10938 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 9 perlcritic violations in C4/TmplTokenType.pm

I just reverted this patch, as it break translator (missed to test it when I tested patches..)

Comment on attachment 10965 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 2 perlcritic violations in C4/Installer/PerlModules.pm

This patch has been reverted = jenkins complain:
perl t/Installer_pm.t 
1..4
ok 1 - use C4::Installer::PerlModules;
ok 2 - The object isa C4::Installer::PerlModules
ok 3 - returns numeric version
not ok 4 - returns correct version
#   Failed test 'returns correct version'
#   at t/Installer_pm.t line 23.
# Looks like you failed 1 test of 4.

After reverting, t/Installer_pm.t is OK

Maybe that's the test that must be upgraded, but until it's done, this patch is reverted

I see at least a couple of patches on this bug which are not signed off. Should the status be changed to "needs signoff?"

hmm, this bug has gotten a bit crazy long...

we should start breaking new perl-critic type patches into smaller manageable bugs, else things can get a little confusing

Should this be 'needs sign-off' for the last 3 patches?
Can the 'signed off' patches be obsoleted? I am not sure if they have been pushed or not.

I rebased these patches.
Some have already fixed.
I removed 1:
 Bug 6679 - [SIGNED-OFF] fix 3 perlcritic violations in C4/ClassSortRoutine.pm
It breaks completely Koha.

Comment on attachment 34197 [details] [review]
Bug 6679: (follow-up) fix 9 perlcritic violations in C4/TmplTokenType.pm

Review of attachment 34197 [details] [review]:
-----------------------------------------------------------------

::: C4/TmplTokenType.pm
@@ +90,5 @@
> +sub DECL              { $_decl }
> +sub PI                { $_pi }
> +sub DIRECTIVE         { $_directive }
> +sub COMMENT           { $_comment }
> +sub UNKNOWN           { $_unknown }

DECL, PI, COMMENT, and UNKNOWN seem to have no easy way to test them. Consider improving testing coverage for TmplTokenType explicitly.

(In reply to M. Tompsett from comment #209)
> Comment on attachment 34197 [details] [review] [review]
> Bug 6679: (follow-up) fix 9 perlcritic violations in C4/TmplTokenType.pm
> 
> Review of attachment 34197 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/TmplTokenType.pm
> @@ +90,5 @@
> > +sub DECL              { $_decl }
> > +sub PI                { $_pi }
> > +sub DIRECTIVE         { $_directive }
> > +sub COMMENT           { $_comment }
> > +sub UNKNOWN           { $_unknown }
> 
> DECL, PI, COMMENT, and UNKNOWN seem to have no easy way to test them.
> Consider improving testing coverage for TmplTokenType explicitly.

Hello Mark,
The patch did not modify the contain of these subroutines, I don't think tests should be mandatory here.
And I don't have any ideas how I could test them :)

Patches pushed to master.

Thanks Mason!

I reverted this patchset. It completely broke Koha.
I'm not sure why it wasn't catched by people who tested it, but my first guess is Perl version, because the semantics for 'eval' might change from one version to another. I haven't checked it on Perl changes.

The page for eval [1] states that this two lines have different semantic meanings:

 - eval $x;     # CASE 1
 - eval { $x }; # CASE 4

which is one of the changes made in several places (notably breaks C4/Installer/PerlDependencies.pm and C4/Dates.pm)

I'll mark it failed QA. My first guess is that we should be either rewriting the hash -> arrayref trick into using anonymous subroutines to do the job, or even refactoring the whole thing (or removing it).

[1] http://perldoc.perl.org/functions/eval.html

This has not been reverted.

Tomas,
I would say you could quickly patch with

diff --git a/C4/Dates.pm b/C4/Dates.pm
index 6ed9588..d7be584 100644
--- a/C4/Dates.pm
+++ b/C4/Dates.pm
@@ -105,7 +105,7 @@ sub dmy_map {
     my $xsub    = $dmy_subs{$dformat};
     $debug and print STDERR "xsub: $xsub \n";
     if ( $val =~ /$re/ ) {
-        my $aref = eval {$xsub};
+        my $aref = eval $xsub;

In a first time.

You can blame me because I get the error too (5.20.1), sorry about that.

Sorry, does not apply

Auto-merging C4/Reports/Guided.pm
CONFLICT (content): Merge conflict in C4/Reports/Guided.pm

(In reply to Chris Cormack from comment #13)
> All pushed, leaving open for me

Given that all the previous patches were likely pushed, and the tweak to C4::Dates is no longer applicable, should set the status to something other than Patch doesn't apply and perhaps obsolete all of these?

Patches rebased, changes to C4::Dates removed.

Back to needs signoff

Comment on attachment 47090 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 3 perlcritic violations in C4/Record.pm

Review of attachment 47090 [details] [review]:
-----------------------------------------------------------------

::: C4/Record.pm
@@ +380,4 @@
>      }
>  
>      # Preprocessing
> +    eval {$preprocess} if ($preprocess);

post-fixed 'if' logic prevents a higher level of perlcritic, doesn't it?

"post-fixed if"
You mean the use of ternary operators?
If yes, please source, I have never heard about that.

(In reply to Jonathan Druart from comment #234)
> "post-fixed if"
> You mean the use of ternary operators?
> If yes, please source, I have never heard about that.

http://search.cpan.org/dist/Perl-Critic-1.126/lib/Perl/Critic/Policy/ControlStructures/ProhibitPostfixControls.pm

We have a massive usage of ternary operators, it won't violate our perlcritic policy.

FWIW, somewhere in the distant past we had a discussion regarding how strict to enforce Conway-style on our code. As I recall, there was a general consensus at that time to apply only limited requirements. As Jonathan's observation points out: increasing style strictness may require a lot of refactoring.

(In reply to Jonathan Druart from comment #236)
> We have a massive usage of ternary operators, it won't violate our
> perlcritic policy.

While true, it's easier to fix it while working on it piece by piece rather than try to refactor massively later. No biggie. It's not something worth failing this over. :)

C4::Record unused variables.
Run koha qa test tools -v 2 -c 5.
perlcritic -5 C4/Record.pm

Comment on attachment 47090 [details] [review]
Bug 6679 - [SIGNED-OFF] fix 3 perlcritic violations in C4/Record.pm

Let's say we abandone this one...

Here is my testing output. It should be noted that patches 1 and 2 are not required for level 5 testing. However, they are necessary for higher levels. It should also be noted that I added:
    [InputOutput::RequireCheckedSyscalls]
    functions = :builtins
    exclude_functions = print
to the perlcriticrc, so I wouldn't have to do stupidity like:
    my $ignore = print "Hello World!";
But that should be a patch to the qa test tools, not Koha itself.

mtompset@debian:~/kohaclone$ git reset --hard origin/master
HEAD is now at 2505a3d Bug 15607: batch patron mod - do not update dates if not                                                                              given
mtompset@debian:~/kohaclone$ perlcritic -5 C4/Installer/PerlModules.pm
C4/Installer/PerlModules.pm source OK
mtompset@debian:~/kohaclone$ perlcritic -5 C4/TmplToken.pm
C4/TmplToken.pm source OK
mtompset@debian:~/kohaclone$ perlcritic -5 C4/TmplTokenType.pm
Subroutine prototypes used at line 86, column 1.  See page 194 of PBP.  (Severit                                                                             y: 5)
Subroutine prototypes used at line 87, column 1.  See page 194 of PBP.  (Severit                                                                             y: 5)
Subroutine prototypes used at line 88, column 1.  See page 194 of PBP.  (Severit                                                                             y: 5)
Subroutine prototypes used at line 89, column 1.  See page 194 of PBP.  (Severit                                                                             y: 5)
Subroutine prototypes used at line 90, column 1.  See page 194 of PBP.  (Severit                                                                             y: 5)
Subroutine prototypes used at line 91, column 1.  See page 194 of PBP.  (Severit                                                                             y: 5)
Subroutine prototypes used at line 92, column 1.  See page 194 of PBP.  (Severit                                                                             y: 5)
Subroutine prototypes used at line 93, column 1.  See page 194 of PBP.  (Severit                                                                             y: 5)
Subroutine prototypes used at line 94, column 1.  See page 194 of PBP.  (Severit                                                                             y: 5)
mtompset@debian:~/kohaclone$ perlcritic -5 opac/ilsdi.pl
Stricture disabled at line 220, column 17.  See page 429 of PBP.  (Severity: 5)
mtompset@debian:~/kohaclone$ git reset --hard origin/master
HEAD is now at 2505a3d Bug 15607: batch patron mod - do not update dates if not given
mtompset@debian:~/kohaclone$ perlcritic -5 C4/Installer/PerlModules.pm
C4/Installer/PerlModules.pm source OK
mtompset@debian:~/kohaclone$ perlcritic -5 C4/TmplToken.pm
C4/TmplToken.pm source OK
mtompset@debian:~/kohaclone$ perlcritic -5 C4/TmplTokenType.pm
Subroutine prototypes used at line 86, column 1.  See page 194 of PBP.  (Severity: 5)
Subroutine prototypes used at line 87, column 1.  See page 194 of PBP.  (Severity: 5)
Subroutine prototypes used at line 88, column 1.  See page 194 of PBP.  (Severity: 5)
Subroutine prototypes used at line 89, column 1.  See page 194 of PBP.  (Severity: 5)
Subroutine prototypes used at line 90, column 1.  See page 194 of PBP.  (Severity: 5)
Subroutine prototypes used at line 91, column 1.  See page 194 of PBP.  (Severity: 5)
Subroutine prototypes used at line 92, column 1.  See page 194 of PBP.  (Severity: 5)
Subroutine prototypes used at line 93, column 1.  See page 194 of PBP.  (Severity: 5)
Subroutine prototypes used at line 94, column 1.  See page 194 of PBP.  (Severity: 5)
mtompset@debian:~/kohaclone$ perlcritic -5 opac/ilsdi.pl
Stricture disabled at line 220, column 17.  See page 429 of PBP.  (Severity: 5)
mtompset@debian:~/kohaclone$ git bz apply 6679
Bug 6679 - Fixing code so it passes basic Perl::Critic tests

47086 - Bug 6679 - [SIGNED-OFF] fix 9 perlcritic violations in C4/TmplTokenType.pm
47087 - Bug 6679 - [SIGNED-OFF] fix 2 perlcritic violations in C4/Installer/PerlModules.pm
47088 - Bug 6679 - Fixing code so it passes basic Perl:::Critic tests
47089 - Bug 6679: (follow-up) fix 9 perlcritic violations in C4/TmplTokenType.pm

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 6679 - [SIGNED-OFF] fix 9 perlcritic violations in C4/TmplTokenType.pm
Applying: Bug 6679 - [SIGNED-OFF] fix 2 perlcritic violations in C4/Installer/PerlModules.pm
Applying: Bug 6679 - Fixing code so it passes basic Perl:::Critic tests
Applying: Bug 6679: (follow-up) fix 9 perlcritic violations in C4/TmplTokenType.pm
mtompset@debian:~/kohaclone$ perlcritic -5 C4/Installer/PerlModules.pm
C4/Installer/PerlModules.pm source OK
mtompset@debian:~/kohaclone$ perlcritic -5 C4/TmplToken.pm
C4/TmplToken.pm source OK
mtompset@debian:~/kohaclone$ perlcritic -5 C4/TmplTokenType.pm
C4/TmplTokenType.pm source OK
mtompset@debian:~/kohaclone$ perlcritic -5 opac/ilsdi.pl
opac/ilsdi.pl source OK
mtompset@debian:~/kohaclone$ ls -la ~/.perlcriticrc
lrwxrwxrwx 1 mtompset mtompset 26 Apr 27  2015 /home/mtompset/.perlcriticrc -> qa-test-tools/perlcriticrc
mtompset@debian:~/kohaclone$ cat ~/qa-test-tools/perlcriticrc
severity = 5
include = ProhibitUnusedVariables
exclude = Miscellanea::RequireRcsKeywords


[Perl::Critic::Policy::BuiltinFunctions::ProhibitStringyEval]
allow_includes =1

[TestingAndDebugging::RequireUseStrict]
equivalent_modules = Modern::Perl

[TestingAndDebugging::RequireUseWarnings]
equivalent_modules = Modern::Perl

[-Modules::RequireBarewordIncludes]

[InputOutput::RequireCheckedSyscalls]
functions = :builtins
exclude_functions = print
mtompset@debian:~/kohaclone$

I agreed with comment #240 regarding C4/Record.pm
Looks good to me, setting to signed off.

Created attachment 47205 [details] [review]
[PASSED QA] Bug 6679 - [SIGNED-OFF] fix 9 perlcritic violations in C4/TmplTokenType.pm

- Subroutine prototypes used at line 86, column 1.  See page 194 of PBP.  (Severity: 5)

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 47206 [details] [review]
[PASSED QA] Bug 6679 - [SIGNED-OFF] fix 2 perlcritic violations in C4/Installer/PerlModules.pm

Expression form of "eval" at line 71, column 9.  See page 161 of PBP.  (Severity: 5)

Expression form of "eval" at line 85, column 13.  See page 161 of PBP.  (Severity: 5)

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 47207 [details] [review]
[PASSED QA] Bug 6679 - Fixing code so it passes basic Perl:::Critic tests

opac/ilsdi.pl: Stricture disabled at line 217, column 17.  See page 429 of PBP.  (Severity: 5)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 47208 [details] [review]
[PASSED QA] Bug 6679: (follow-up) fix 9 perlcritic violations in C4/TmplTokenType.pm

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Pushed to Master - Should be in the May 2016 release. Thanks!

Patch pushed to 3.22.x, will be in 3.22.3

(In reply to Julian Maurice from comment #252)
> Patch pushed to 3.22.x, will be in 3.22.3

Watch out for bug 15680.

This patch has been pushed to 3.20.x, will be in 3.20.9.