|
Description
Marcel de Rooy
2016-10-14 12:17:31 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! 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 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. (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. 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> 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> 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 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. :) 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> 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> 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> 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> 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> (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. 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> 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> 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> 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> Pushed to master for 17.05, thanks Marcel, Mark! This looks like code clean-up - treating this as an enhancement. 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. Which bug does it fix? If there is a test plan to verify it fixes an existing bug, I might reconsider. |