After a hiatus of several months and a discussion with particle and
chromatic at YAPC, I have resumed work on testing of the Parrot
configuration step classes.
One of the benefits of this work is that it guarantees that there is >=
1 person who has actually looked at the code of each step class in the
last year. Classes where the Perl 5 code is, shall we say, less than
optimal get an overdue refactoring.
And that's the case with this class, auto::icu. From 'svn log' it
appears that its essentials have not been touched in a long time -- and
it shows. I've started the 'autoicu' branch in SVN to hold code while
we refactor and test.
Two problems I have with the code right off the bat are:
1. Variables unwisely multi-purposed. Five variables are declared
inside the step's runstep() method to hold the give possible
command-line options:
my ( $verbose, $icushared, $icuheaders, $icuconfig, $without ) =
$conf->options->get( qw|
verbose
icushared
icuheaders
icu-config
without-icu
| );
Now, IMO such variables should be treated as read-only, i.e., throughout
the duration of the runstep() method, these variables should always be
available to report on what the user selected on the command-line -- and
they should *only* report on those user selections. They should not be
*assigned to*, since if they were they would no longer be able to report
what the user selected.
But in this step's runstep(), several of these variables are assigned to
to hold information obtained during the configuration process. So, at a
given point it can be difficult to say what kind of information a
particular variable is holding.
Example:
2. Too many 'if' blocks which lack corresponding 'else' branches. Add
to this the fact that many of the 'ifs' and 'unlesses' are written in
idiomatic Perl (i.e., at the end of statements), the flow of runstep()
is not easily *skimmable* (cf. Schwern's talk at YAPC).
Example:
To deal with this, in the 'autoicu' branch I have explictly coded all
the 'else' branches (except for 'verbose') -- just so that I can get a
better sense of what's going on.
Since heretofore I have not configured with ICU on either Darwin or
Linux, I haven't developed a good sense of what this config step does.
I'll probably be installing ICU on one of those systems but not the other.
In the meantime, I would appreciate hearing from any Parrot developer
who customarily sets any of the 'icushared', 'icuheaders' or
'icu-config' options on the command line. If you fall into that
category, I would like to ask you to test this step as I work on it in
the 'autoicu' branch.
Thank you very much.
kid51
