Bug 15871 - Improve perl critic of t/RecordProcessor.t
Summary: Improve perl critic of t/RecordProcessor.t
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: M. Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on: 15777
Blocks: 15870
  Show dependency treegraph
 
Reported: 2016-02-19 21:19 UTC by M. Tompsett
Modified: 2017-06-14 22:08 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15871: Improve PerlCritic level for t/RecordProcessor.t (11.54 KB, patch)
2016-02-19 21:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 15871: Improve PerlCritic level for t/RecordProcessor.t (11.81 KB, patch)
2016-02-19 22:01 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 15871: Improve PerlCritic level for t/RecordProcessor.t (11.88 KB, patch)
2016-02-24 04:24 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 15871: Improve PerlCritic level for t/RecordProcessor.t (11.88 KB, patch)
2016-02-24 04:28 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 15871: Improve PerlCritic level for t/RecordProcessor.t (11.95 KB, patch)
2016-02-24 15:27 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15871: Improve PerlCritic level for t/RecordProcessor.t (12.02 KB, patch)
2016-02-24 16:13 UTC, Jesse Maseto
Details | Diff | Splinter Review
Bug 15871: Improve PerlCritic level for t/RecordProcessor.t (12.33 KB, patch)
2016-03-03 15:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description M. Tompsett 2016-02-19 21:19:16 UTC
perlcritic -5 t/RecordProcessor fails with:
Don't modify $_ in list functions at line 43, column 25.  See page 114 of PBP.  (Severity: 5)
This is because of a hacky good single line of code.

While correcting this for improved readability, attempt to get this to a higher level of perl critic.
Comment 1 M. Tompsett 2016-02-19 21:23:41 UTC Comment hidden (obsolete)
Comment 2 M. Tompsett 2016-02-19 22:01:36 UTC Comment hidden (obsolete)
Comment 3 M. Tompsett 2016-02-24 02:26:15 UTC
Marked as trivial rather than an enhancement, because technically, the failure of perlcritic level 5 would mean it wouldn't pass koha qa test tools.
Comment 4 M. Tompsett 2016-02-24 04:24:22 UTC Comment hidden (obsolete)
Comment 5 M. Tompsett 2016-02-24 04:28:43 UTC Comment hidden (obsolete)
Comment 6 M. Tompsett 2016-02-24 04:31:10 UTC
Cleaned up after a rebase. Ready for signoff. :)
Comment 7 Héctor Eduardo Castro Avalos 2016-02-24 15:27:33 UTC Comment hidden (obsolete)
Comment 8 Jesse Maseto 2016-02-24 16:13:01 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2016-03-03 15:48:59 UTC
Created attachment 48631 [details] [review]
Bug 15871: Improve PerlCritic level for t/RecordProcessor.t

perlcritic -5 failed.
Attempt to clean up to a higher level:
-- use English to address use of $@ variable
-- perltidy on the code
-- substitute q{} for ''
-- expand out single line hacky goodness (... s/\.pm$//) to more code
-- remove parenthesis on functions that don't need it
-- add x, s, and m as needed to regexps
-- change double quotes to single quotes where no variable involved
-- tweaked eval destroy test to check return value and use $EVAL_ERROR
-- renamed $processor to $record_processor in the subtest to avoid
   lexical warnings

TEST PLAN
---------

$ perlcritic -5 t/RecordProcessor.t
Don't modify $_ in list functions at line 43, column 25.  See page 114 of PBP.  (Severity: 5)

$ perlcritic -2 t/RecordProcessor.t
No package-scoped "$VERSION" variable found at line 1, column 1.  See page 404 of PBP.  (Severity: 2)
Quotes used with a string containing no non-whitespace characters at line 34, column 36.  See page 53 of PBP.  (Severity: 2)
Quotes used with a string containing no non-whitespace characters at line 34, column 39.  See page 53 of PBP.  (Severity: 2)
Quotes used with a string containing no non-whitespace characters at line 36, column 33.  See page 53 of PBP.  (Severity: 2)
Quotes used with a string containing no non-whitespace characters at line 36, column 36.  See page 53 of PBP.  (Severity: 2)
Don't modify $_ in list functions at line 43, column 25.  See page 114 of PBP.  (Severity: 5)
Regular expression without "/s" flag at line 43, column 33.  See pages 240,241 of PBP.  (Severity: 2)
Regular expression without "/x" flag at line 43, column 33.  See page 236 of PBP.  (Severity: 3)
Regular expression without "/m" flag at line 43, column 33.  See page 237 of PBP.  (Severity: 2)
Regular expression without "/s" flag at line 43, column 66.  See pages 240,241 of PBP.  (Severity: 2)
Regular expression without "/x" flag at line 43, column 66.  See page 236 of PBP.  (Severity: 3)
Regular expression without "/m" flag at line 43, column 66.  See page 237 of PBP.  (Severity: 2)
Expression form of "grep" at line 47, column 8.  See page 169 of PBP.  (Severity: 4)
Expression form of "grep" at line 50, column 20.  See page 169 of PBP.  (Severity: 4)
Regular expression without "/s" flag at line 50, column 26.  See pages 240,241 of PBP.  (Severity: 2)
Regular expression without "/m" flag at line 50, column 26.  See page 237 of PBP.  (Severity: 2)
Return value of eval not tested at line 73, column 1.  You can't depend upon the value of $@/$EVAL_ERROR to tell whether an eval failed.  (Severity: 3)
Magic punctuation variable $@ used at line 78, column 5.  See page 79 of PBP.  (Severity: 2)
Reused variable name in lexical scope: $processor at line 84, column 5.  Invent unique variable names.  (Severity: 3)
Subroutine "new" called using indirect syntax at line 87, column 18.  See page 349 of PBP.  (Severity: 4)
Subroutine "new" called using indirect syntax at line 93, column 18.  See page 349 of PBP.  (Severity: 4)
Quotes used with a string containing no non-whitespace characters at line 96, column 40.  See page 53 of PBP.  (Severity: 2)
Subroutine "new" called using indirect syntax at line 99, column 18.  See page 349 of PBP.  (Severity: 4)
Subroutine "new" called using indirect syntax at line 106, column 18.  See page 349 of PBP.  (Severity: 4)
$ prove -v t/RecordProcessor.t
t/RecordProcessor.t .. ok
All tests successful.
Files=1, Tests=13,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.22 cusr  0.02 csys =  0.25 CPU)
Result: PASS

$ prove -v t/RecordProcessor.t
...
$ git bz apply 15871

Repeat perlcritic level 2, and only $VERSION warning should exist.
Retest with the prove.
Run koha qa test tools.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as advertised

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
I don't really care about perlcritic as long as it involves changing '' into qw{} (WTF?)
Anyway, I'd do this kind of things as we go, for example, if we were adding more tests. In that
case it would just be a followup for this, after you provided a patch for an enh/bugfix.
Comment 10 Brendan Gallagher 2016-03-03 22:11:07 UTC
Pushed to Master - Should be in the May 2016 Release.  Thanks!
Comment 11 Julian Maurice 2016-03-07 13:27:49 UTC
Patch pushed to 3.22.x, will be in 3.22.5