Bug 17447 - Remove unused vars from batchRebuildItemsTables.pl
Summary: Remove unused vars from batchRebuildItemsTables.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-14 12:17 UTC by Marcel de Rooy
Modified: 2017-12-07 22:18 UTC (History)
3 users (show)

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


Attachments
Bug 17447: Housekeeping: Tidy up misc/batchRebuildItemsTables.pl (6.74 KB, patch)
2016-10-14 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17447: Housekeeping: Tidy up misc/batchRebuildItemsTables.pl (6.76 KB, patch)
2016-10-17 08:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17447: Run perltidy on batchRebuildItemsTables.pl (6.10 KB, patch)
2016-12-30 10:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17447: Remove unused vars from batchRebuildItemsTables.pl (2.73 KB, patch)
2016-12-30 10:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17447: %opt is unused (945 bytes, patch)
2016-12-30 15:13 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17447: Run perltidy on batchRebuildItemsTables.pl (6.12 KB, patch)
2016-12-30 15:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17447: Remove unused vars from batchRebuildItemsTables.pl (2.74 KB, patch)
2016-12-30 15:41 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17447: Run perltidy on batchRebuildItemsTables.pl (6.12 KB, patch)
2016-12-30 15:45 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17447: Remove unused vars from batchRebuildItemsTables.pl (2.74 KB, patch)
2016-12-30 15:45 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17447: %opt is unused (1.13 KB, patch)
2016-12-30 15:45 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17447: %opt is unused (1.23 KB, patch)
2016-12-30 16:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17447: Run perltidy on batchRebuildItemsTables.pl (6.17 KB, patch)
2016-12-30 18:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17447: Remove unused vars from batchRebuildItemsTables.pl (2.79 KB, patch)
2016-12-30 18:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17447: %opt is unused (1.24 KB, patch)
2016-12-30 18:00 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2016-10-14 12:17:31 UTC

    
Comment 1 Marcel de Rooy 2016-10-14 12:35:08 UTC
Created attachment 56516 [details] [review]
Bug 17447: Housekeeping: Tidy up misc/batchRebuildItemsTables.pl

This script contained lots of complete if blocks on one line.
In this patch a lot of whitespace has been moved around.
A line with a call to _build_tag_directory has been removed; the results
were discarded.
Variable $extkey was not used either.
The script now does not only print help if you specify -t.

Test plan:
Run the script. Do not test on production data!
Comment 2 Marcel de Rooy 2016-10-17 08:02:24 UTC
Created attachment 56578 [details] [review]
Bug 17447: Housekeeping: Tidy up misc/batchRebuildItemsTables.pl

This script contained lots of complete if blocks on one line.
In this patch a lot of whitespace has been moved around.
A line with a call to _build_tag_directory has been removed; the results
were discarded.
Variable $extkey was not used either.
The script now does not only print help if you specify -t.

Test plan:
Run the script. Do not test on production data!
Comment 3 Mark Tompsett 2016-12-22 19:29:58 UTC
Comment on attachment 56578 [details] [review]
Bug 17447: Housekeeping: Tidy up misc/batchRebuildItemsTables.pl

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

::: misc/batchRebuildItemsTables.pl
@@ +35,3 @@
>  my $starttime = gettimeofday;
>  my @errors;
>  my %opt;

Is this even used?

@@ +43,4 @@
>      'where:s' => \$where,
>  ) or pod2usage(2);
>  
> +pod2usage(1) if $help || ( !$confirm && !$test_parameter );

Tweaks and whitespace changes should be separate commits to make it easier to check.
git diff -w origin/master pointed out this and a couple others.
Comment 4 Marcel de Rooy 2016-12-30 10:24:04 UTC
(In reply to M. Tompsett from comment #3)
> >  my $starttime = gettimeofday;
> >  my @errors;
> >  my %opt;
> 
> Is this even used?
The first two are used, the third is not. Removing it now too.

> Tweaks and whitespace changes should be separate commits to make it easier
> to check.
So true :) Added a perltidy patch.
Comment 5 Marcel de Rooy 2016-12-30 10:25:47 UTC
Created attachment 58523 [details] [review]
Bug 17447: Run perltidy on batchRebuildItemsTables.pl

Test plan:
Verify that the output of diff -w between the original and tidied file
does not introduce code changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2016-12-30 10:26:21 UTC
Created attachment 58524 [details] [review]
Bug 17447: Remove unused vars from batchRebuildItemsTables.pl

Variables $extkey not used, %opt not used.
Variables $tmptest[...] not used, calling _build_tag_directory useless.
The script now does not only print help if you specify -t.
Sub defnonull tidied.
Rearranged modules, removed Dumper.

Test plan:
[1] Run the script with -t flag. The script should not only print usage
    statement, but should do a dry run. (Test this on a small database,
    or pass an additional where clause.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Mark Tompsett 2016-12-30 15:13:48 UTC
Created attachment 58528 [details] [review]
Bug 17447: %opt is unused

Deleted the line.
perlcritic -4 before and after. patch will have one less difference.
Comment 8 Mark Tompsett 2016-12-30 15:37:58 UTC
Comment on attachment 58528 [details] [review]
Bug 17447: %opt is unused

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

::: misc/batchRebuildItemsTables.pl
@@ +109,4 @@
>  
>  sub defnonull {
>      my $var = shift;
> +    return defined $var && $var ne q{};

This was so I could get perlcritic -4. :)
Comment 9 Mark Tompsett 2016-12-30 15:40:54 UTC
Created attachment 58529 [details] [review]
Bug 17447: Run perltidy on batchRebuildItemsTables.pl

Test plan:
Verify that the output of diff -w between the original and tidied file
does not introduce code changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 10 Mark Tompsett 2016-12-30 15:41:28 UTC
Created attachment 58530 [details] [review]
Bug 17447: Remove unused vars from batchRebuildItemsTables.pl

Variables $extkey not used, %opt not used.
Variables $tmptest[...] not used, calling _build_tag_directory useless.
The script now does not only print help if you specify -t.
Sub defnonull tidied.
Rearranged modules, removed Dumper.

Test plan:
[1] Run the script with -t flag. The script should not only print usage
    statement, but should do a dry run. (Test this on a small database,
    or pass an additional where clause.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

NOTE: Fixed %opt in my followup.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 11 Mark Tompsett 2016-12-30 15:45:15 UTC
Created attachment 58531 [details] [review]
Bug 17447: Run perltidy on batchRebuildItemsTables.pl

Test plan:
Verify that the output of diff -w between the original and tidied file
does not introduce code changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 12 Mark Tompsett 2016-12-30 15:45:23 UTC
Created attachment 58532 [details] [review]
Bug 17447: Remove unused vars from batchRebuildItemsTables.pl

Variables $extkey not used, %opt not used.
Variables $tmptest[...] not used, calling _build_tag_directory useless.
The script now does not only print help if you specify -t.
Sub defnonull tidied.
Rearranged modules, removed Dumper.

Test plan:
[1] Run the script with -t flag. The script should not only print usage
    statement, but should do a dry run. (Test this on a small database,
    or pass an additional where clause.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 13 Mark Tompsett 2016-12-30 15:45:28 UTC
Created attachment 58533 [details] [review]
Bug 17447: %opt is unused

Deleted the line.
perlcritic -4 before and after.
Before there are issues. After there is not.

Also, changed function to not rely on implicit return value
of last line, but explicitly stated a return. And operator
changed, due to precedence issues.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 14 Marcel de Rooy 2016-12-30 16:17:57 UTC
(In reply to M. Tompsett from comment #13)
> Created attachment 58533 [details] [review] [review]
> Bug 17447: %opt is unused
> 
> Deleted the line.
> perlcritic -4 before and after.
> Before there are issues. After there is not.
> 
> Also, changed function to not rely on implicit return value
> of last line, but explicitly stated a return. And operator
> changed, due to precedence issues.
> 
> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Thx Mark.
I am sure to have deleted the opt line, but pretty sure now too that i did not commit that last change ;)
The change in defnonull is fine with me, but the QAer may also remove it completely. We could just say something like pref(merge) || 0.
Comment 15 Marcel de Rooy 2016-12-30 16:18:19 UTC
Created attachment 58534 [details] [review]
Bug 17447: %opt is unused

Deleted the line.
perlcritic -4 before and after.
Before there are issues. After there is not.

Also, changed function to not rely on implicit return value
of last line, but explicitly stated a return. And operator
changed, due to precedence issues.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Kyle M Hall 2016-12-30 18:00:23 UTC
Created attachment 58537 [details] [review]
Bug 17447: Run perltidy on batchRebuildItemsTables.pl

Test plan:
Verify that the output of diff -w between the original and tidied file
does not introduce code changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2016-12-30 18:00:33 UTC
Created attachment 58538 [details] [review]
Bug 17447: Remove unused vars from batchRebuildItemsTables.pl

Variables $extkey not used, %opt not used.
Variables $tmptest[...] not used, calling _build_tag_directory useless.
The script now does not only print help if you specify -t.
Sub defnonull tidied.
Rearranged modules, removed Dumper.

Test plan:
[1] Run the script with -t flag. The script should not only print usage
    statement, but should do a dry run. (Test this on a small database,
    or pass an additional where clause.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2016-12-30 18:00:36 UTC
Created attachment 58539 [details] [review]
Bug 17447: %opt is unused

Deleted the line.
perlcritic -4 before and after.
Before there are issues. After there is not.

Also, changed function to not rely on implicit return value
of last line, but explicitly stated a return. And operator
changed, due to precedence issues.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2017-01-13 11:36:52 UTC
Pushed to master for 17.05, thanks Marcel, Mark!
Comment 20 Katrin Fischer 2017-01-15 17:22:56 UTC
This looks like code clean-up - treating this as an enhancement.
Comment 21 Mark Tompsett 2017-01-16 02:09:37 UTC
Comment on attachment 58538 [details] [review]
Bug 17447: Remove unused vars from batchRebuildItemsTables.pl

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

::: misc/batchRebuildItemsTables.pl
@@ +43,4 @@
>      'where:s' => \$where,
>  ) or pod2usage(2);
>  
> +pod2usage(1) if $help || ( !$confirm && !$test_parameter );

This is the only reason you would consider it an enhancement, but it behaves more like a bug fix.
Comment 22 Katrin Fischer 2017-01-16 09:04:55 UTC
Which bug does it fix? If there is a test plan to verify it fixes an existing bug, I might reconsider.