Bug 21395 - Make perlcritic happy
Summary: Make perlcritic happy
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
: 25874 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-09-21 16:14 UTC by Julian Maurice
Modified: 2022-06-06 20:25 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.02


Attachments
Bug 21395: Make perlcritic happy (124.23 KB, patch)
2018-09-21 16:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 21395: Make perlcritic happy (121.32 KB, patch)
2020-04-24 11:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 21395: Fix QA errors (6.64 KB, patch)
2020-04-27 09:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 21395: Make perlcritic happy (121.38 KB, patch)
2020-04-30 14:28 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 21395: Fix QA errors (6.70 KB, patch)
2020-04-30 14:28 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 21395: Make perlcritic happy (121.50 KB, patch)
2020-06-25 14:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21395: Fix QA errors (6.76 KB, patch)
2020-06-25 14:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21395: (QA follow-up) Remove some introduced issues (9.65 KB, patch)
2020-06-25 14:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21395: (QA follow-up) POD fixes (2.98 KB, patch)
2020-06-25 14:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21395: Remove 'variable $DEBUG masks earlier declaration in same scope' warning (6.42 KB, patch)
2020-06-29 11:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21395: Fix creation of PO file (776 bytes, patch)
2020-06-29 13:00 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 21395: Ignore .perlcriticrc in t/Makefile.t (559 bytes, patch)
2020-06-29 13:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 21395: Fix C4/Barcodes/ValueBuilder.pm (5.90 KB, patch)
2020-06-29 13:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 21395: Fix misc/admin/koha-preferences (758 bytes, patch)
2020-06-29 13:24 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2018-09-21 16:14:14 UTC

    
Comment 1 Julian Maurice 2018-09-21 16:16:12 UTC
Created attachment 79239 [details] [review]
Bug 21395: Make perlcritic happy

This patch adds a .perlcriticrc (copied from qa-test-tools) and fixes
almost all perlcrictic violations according to this .perlcriticrc
The remaining violations are silenced out by appending a '## no critic'
to the offending lines. They can still be seen by using the --force
option of perlcritic
This patch also modify t/00-testcritic.t to check all Perl files using
the new .perlcriticrc.
I'm not sure if this test script is still useful as it is now equivalent
to `perlcritic --quiet .` and it looks like it is much slower
(approximatively 5 times slower on my machine)

Test plan:
1. Run `perlcritic --quiet .` from the root directory. It should output
   nothing
2. Run `perlcritic --quiet --force .`. It should output 6 errors (5
   StringyEval, 1 BarewordFileHandles)
3. Run `TEST_QA=1 prove t/00-testcritic.t`
4. Read the patch. Check that all changes make sense and do not
   introduce undesired behaviour
Comment 2 Julian Maurice 2020-04-24 11:41:27 UTC
Created attachment 103666 [details] [review]
Bug 21395: Make perlcritic happy

This patch adds a .perlcriticrc (copied from qa-test-tools) and fixes
almost all perlcrictic violations according to this .perlcriticrc
The remaining violations are silenced out by appending a '## no critic'
to the offending lines. They can still be seen by using the --force
option of perlcritic
This patch also modify t/00-testcritic.t to check all Perl files using
the new .perlcriticrc.
I'm not sure if this test script is still useful as it is now equivalent
to `perlcritic --quiet .` and it looks like it is much slower
(approximatively 5 times slower on my machine)

Test plan:
1. Run `perlcritic --quiet .` from the root directory. It should output
   nothing
2. Run `perlcritic --quiet --force .`. It should output 7 errors (6
   StringyEval, 1 BarewordFileHandles)
3. Run `TEST_QA=1 prove t/00-testcritic.t`
4. Read the patch. Check that all changes make sense and do not
   introduce undesired behaviour
Comment 3 Bernardo Gonzalez Kriegel 2020-04-25 14:30:58 UTC
Hi Julian,
an extensive change!

All your test pass (1,2,3) but qa tools reports some errors, a lot of them are from tab chars, bout others are more real

For example
FAIL   misc/migration_tools/22_to_30/export_Authorities_xml.pl
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   OK     spelling
   FAIL   valid
                Global symbol "$time" requires explicit package name (did you forget to declare "my $time"?) 
                Global symbol "$string" requires explicit package name (did you forget to declare "my $string"?) 
                misc/migration_tools/22_to_30/export_Authorities_xml.pl had compilation errors.

this is because of a missing ';' at the end of this line 
34	    my $string= ($time=~m/([0-9\-]+)/) ? $1 : undef

Others need exploring.
Comment 4 Julian Maurice 2020-04-27 09:02:08 UTC
Created attachment 103740 [details] [review]
Bug 21395: Fix QA errors
Comment 5 Julian Maurice 2020-04-27 09:02:59 UTC
I did not fix 'tab chars' errors, but all other errors should be gone
Comment 6 Bernardo Gonzalez Kriegel 2020-04-30 14:28:53 UTC
Created attachment 104038 [details] [review]
Bug 21395: Make perlcritic happy

This patch adds a .perlcriticrc (copied from qa-test-tools) and fixes
almost all perlcrictic violations according to this .perlcriticrc
The remaining violations are silenced out by appending a '## no critic'
to the offending lines. They can still be seen by using the --force
option of perlcritic
This patch also modify t/00-testcritic.t to check all Perl files using
the new .perlcriticrc.
I'm not sure if this test script is still useful as it is now equivalent
to `perlcritic --quiet .` and it looks like it is much slower
(approximatively 5 times slower on my machine)

Test plan:
1. Run `perlcritic --quiet .` from the root directory. It should output
   nothing
2. Run `perlcritic --quiet --force .`. It should output 7 errors (6
   StringyEval, 1 BarewordFileHandles)
3. Run `TEST_QA=1 prove t/00-testcritic.t`
4. Read the patch. Check that all changes make sense and do not
   introduce undesired behaviour

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 7 Bernardo Gonzalez Kriegel 2020-04-30 14:28:57 UTC
Created attachment 104039 [details] [review]
Bug 21395: Fix QA errors

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 8 Bernardo Gonzalez Kriegel 2020-04-30 14:34:09 UTC
(In reply to Julian Maurice from comment #5)
> I did not fix 'tab chars' errors, but all other errors should be gone

Ok, only that remains are pre-existing tab chars and a small pod error
Signed
Comment 9 Martin Renvoize 2020-06-25 14:21:58 UTC
Created attachment 106293 [details] [review]
Bug 21395: Make perlcritic happy

This patch adds a .perlcriticrc (copied from qa-test-tools) and fixes
almost all perlcrictic violations according to this .perlcriticrc
The remaining violations are silenced out by appending a '## no critic'
to the offending lines. They can still be seen by using the --force
option of perlcritic
This patch also modify t/00-testcritic.t to check all Perl files using
the new .perlcriticrc.
I'm not sure if this test script is still useful as it is now equivalent
to `perlcritic --quiet .` and it looks like it is much slower
(approximatively 5 times slower on my machine)

Test plan:
1. Run `perlcritic --quiet .` from the root directory. It should output
   nothing
2. Run `perlcritic --quiet --force .`. It should output 7 errors (6
   StringyEval, 1 BarewordFileHandles)
3. Run `TEST_QA=1 prove t/00-testcritic.t`
4. Read the patch. Check that all changes make sense and do not
   introduce undesired behaviour

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-06-25 14:22:01 UTC
Created attachment 106294 [details] [review]
Bug 21395: Fix QA errors

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2020-06-25 14:22:04 UTC
Created attachment 106295 [details] [review]
Bug 21395: (QA follow-up) Remove some introduced issues

This patch removes some new error cases introduced during rebase

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-06-25 14:22:08 UTC
Created attachment 106296 [details] [review]
Bug 21395: (QA follow-up) POD fixes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-06-25 14:22:46 UTC
Nice work.

Rebased slightly and thoroughly tested. It's passing QA tests and makes allot of sense to me.

Passing QA
Comment 14 Jonathan Druart 2020-06-29 10:31:30 UTC
> This patch also modify t/00-testcritic.t to check all Perl files using
> the new .perlcriticrc.
> I'm not sure if this test script is still useful as it is now equivalent
> to `perlcritic --quiet .` and it looks like it is much slower
> (approximatively 5 times slower on my machine)

Are you sure about that?

% time perlcritic --quiet .
real    2m10.957s

% time prove t/00-testcritic.t
real    1m2.354s
Comment 15 Jonathan Druart 2020-06-29 11:43:47 UTC
Created attachment 106390 [details] [review]
Bug 21395: Remove 'variable $DEBUG masks earlier declaration in same scope' warning

% prove t/db_dependent/Serials.t
t/db_dependent/Serials.t .. 8/49 "my" variable $DEBUG masks earlier declaration in same scope at /kohadevbox/koha/C4/Barcodes/ValueBuilder.pm line 45.
"my" variable $DEBUG masks earlier declaration in same scope at /kohadevbox/koha/C4/Barcodes/ValueBuilder.pm line 87.
Comment 16 Jonathan Druart 2020-06-29 11:50:14 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 17 Julian Maurice 2020-06-29 12:02:27 UTC
(In reply to Jonathan Druart from comment #14)
> > This patch also modify t/00-testcritic.t to check all Perl files using
> > the new .perlcriticrc.
> > I'm not sure if this test script is still useful as it is now equivalent
> > to `perlcritic --quiet .` and it looks like it is much slower
> > (approximatively 5 times slower on my machine)
> 
> Are you sure about that?
> 
> % time perlcritic --quiet .
> real    2m10.957s
> 
> % time prove t/00-testcritic.t
> real    1m2.354s

I just tested again, and I get similar times (t/00-testcritic.t is even faster (~40s)). It looks like t/00-testcritic.t uses multiple CPUs, unlike perlcritic. So t/00-testcritic.t is still useful :)
Comment 18 Jonathan Druart 2020-06-29 12:18:32 UTC
xt/author/translatable-templates.t is hardly failing. I noticed it before the push but I thought it was coming from local changes.

https://jenkins.koha-community.org/job/Koha_Master_U18/831/consoleFull

Can you please fix ASAP?
Comment 19 Jonathan Druart 2020-06-29 12:24:49 UTC
Also I think the changes to the migration_tools must be reverted unless they have been correctly tested.

misc/migration_tools/22_to_30/export_Authorities_xml.pl and misc/migration_tools/22_to_30/move_marc_to_biblioitems.pl
Comment 20 Jonathan Druart 2020-06-29 12:36:31 UTC
And there are some other failures:
 * xt/author/translatable-templates.t
 * t/Makefile.t
 * t/00-testcritic.t
 * t/db_dependent/00-strict.t
Comment 21 Julian Maurice 2020-06-29 13:00:50 UTC
Created attachment 106393 [details] [review]
Bug 21395: Fix creation of PO file
Comment 22 Julian Maurice 2020-06-29 13:02:12 UTC
Created attachment 106394 [details] [review]
Bug 21395: Ignore .perlcriticrc in t/Makefile.t
Comment 23 Julian Maurice 2020-06-29 13:16:00 UTC
Created attachment 106395 [details] [review]
Bug 21395: Fix C4/Barcodes/ValueBuilder.pm

$DEBUG variable was always set to 0, so it was useless
Comment 24 Julian Maurice 2020-06-29 13:24:07 UTC
Created attachment 106398 [details] [review]
Bug 21395: Fix misc/admin/koha-preferences
Comment 25 Jonathan Druart 2020-06-29 13:49:44 UTC
Last four patches pushed to master.
Comment 26 Martin Renvoize 2020-06-29 14:05:12 UTC
Thanks for the quick followups Julian.. great to see this one having progressed.
Comment 27 Andrii Nugged 2020-06-29 19:28:47 UTC Comment hidden (obsolete, wrong_ticket)
Comment 28 Lucas Gass 2020-07-13 17:03:33 UTC
backported to 20.05.x. for 20.05.02
Comment 29 Aleisha Amohia 2020-07-16 04:55:39 UTC
enhancement, not backported to 19.11.x
Comment 30 Jonathan Druart 2020-07-28 09:02:49 UTC
*** Bug 25874 has been marked as a duplicate of this bug. ***