|
Description
Julian Maurice
2018-09-21 16:14:14 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 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 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.
Created attachment 103740 [details] [review] Bug 21395: Fix QA errors I did not fix 'tab chars' errors, but all other errors should be gone 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> Created attachment 104039 [details] [review] Bug 21395: Fix QA errors Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> (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 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> 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> 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> Created attachment 106296 [details] [review] Bug 21395: (QA follow-up) POD fixes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Nice work. Rebased slightly and thoroughly tested. It's passing QA tests and makes allot of sense to me. Passing QA > 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
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. Pushed to master for 20.11, thanks to everybody involved! (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 :) 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? 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 And there are some other failures: * xt/author/translatable-templates.t * t/Makefile.t * t/00-testcritic.t * t/db_dependent/00-strict.t Created attachment 106393 [details] [review] Bug 21395: Fix creation of PO file Created attachment 106394 [details] [review] Bug 21395: Ignore .perlcriticrc in t/Makefile.t Created attachment 106395 [details] [review] Bug 21395: Fix C4/Barcodes/ValueBuilder.pm $DEBUG variable was always set to 0, so it was useless Created attachment 106398 [details] [review] Bug 21395: Fix misc/admin/koha-preferences Last four patches pushed to master. Thanks for the quick followups Julian.. great to see this one having progressed. Small (below trivial :P) postfix: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25896 backported to 20.05.x. for 20.05.02 enhancement, not backported to 19.11.x *** Bug 25874 has been marked as a duplicate of this bug. *** |