Re: [Bug-wget] [PATCH] Stylistic and idiomatic cleanups in Perl tests

[Tim Ruehsen]

Thank you, Pär and Hubert.

A very good catch !

Since none of our Perl tests failed though we have broken Perl code, the 
question arises, if this is dead code (not executed when running our tests) !?

BTW, I would like to mention that we plan to translate the Perl tests into 
Python. In the end, we want to get rid of the Perl test suite.
So, before you start spending your precious developer time into improving the 
Perl test suite, please consider improving the Python test suite (and/or move 
tests from Perl to Python).

Anyways, i'll push your patch soon if nobody complains.

Regards, Tim

On Tuesday 19 May 2015 22:27:24 Pär Karlsson wrote:
> Yes, you are right. The declaration of $i inside the for expression shadows
> the scope of the outer $i inside the for loop (which indeed makes the outer
> $i uninitialized after the loop).
> 
> The for loop itself is not stylistically Perlish with the C-style loop, but
> that's a minor gripe. I made an error when using the Perl-style range
> operator ( N1 .. N2 ) loop and forgot to remove the commented out line too.
> And I mistakenly left the extraneous 'my' there, which declares $i in a new
> scope. After some unintentional off-list conversation with Hubert - which
> was lucky since he discovered another embarrasing mistake - my suggestion
> would be to change it to a while loop instead (see attached patch).
> 
> Best regards,
> 
> /Pär
> 
> 2015-05-19 20:04 GMT+02:00 Hubert Tarasiuk <address@hidden>:
> > > From: Pär Karlsson <address@hidden>
> > > 
> > > A number of Perl-specific cleanups in the test suite perl modules.
> > > 
> > > Most of these changes have no immediate functional difference but puts
> > 
> > code
> > 
> > > in line with the same formatting (perltidy GNU style) for readability.
> > > 
> > > A warning regarding invalid operators on incompatible values (numeric vs
> > > strings) have been fixed.
> > > 
> > > Some common but discouraged Perl idioms have been updated to a somewhat
> > > clearer style, especially regarding evals and map() vs. for loops.
> > > 
> > > I did not touch the FTP and HTTP server modules functionally, but there
> > 
> > seem
> > 
> > > to be a lot of strange code there in, and would warrant further cleanups
> > > and investigations if these tests would remain in Perl, instead of
> > > moving
> > > over to the Python based tests.
> > > 
> > > All tests work nominally on my machine (with perl-5.20.1) but should be
> > > compatible with versions down to 5.6.
> > 
> > I believe I have found an issue with WgetTests module:
> > >     my $i;
> > >     
> > >     # for ($i=0; $i != $min; ++$i) {
> > >     for my $i (0 .. $min - 1)
> > >     {
> > >     
> > >         last if substr($expected, $i, 1) ne substr $actual, $i, 1;
> > >         if (substr($expected, $i, 1) eq q{\n})
> > >         {
> > >         
> > >             $line++;
> > >             $col = 0;
> > >         
> > >         }
> > >         else
> > >         {
> > >         
> > >             $col++;
> > >         
> > >         }
> > >     
> > >     }
> > >     my $snip_start = $i - ($SNIPPET_SIZE / 2);
> > >     if ($snip_start < 0)
> > >     {
> > >     
> > >         $SNIPPET_SIZE += $snip_start;    # Take it from the end.
> > >         $snip_start = 0;
> > >     
> > >     }
> > 
> > I am no Perl expert but I tested it and current for loop will not set
> > the $i variable to any value (outside the loop). (Actually it gives me
> > warning about using uninitialized variable when calculating $snip_start
> > and the diff is not printed correctly.)
> > Reverting the commented version seems to fix this problem. Removing the
> > "my" from current for loop does not seem to fix the problem.
> > --
> > Hubert Tarasiuk

signature.asc
Description: This is a digitally signed message part.