Elizabeth:
I think the question you ask in your subject line is the most important
aspect of your posting. Over the past month I've gotten errors from
time to time when running tests which call Parrot::Distribution. This
has led me to poke around inside lib/Parrot/Distribution.pm and be
puzzled by a lot of what I see there. I think the solution to that
puzzle will only come when we have a clear answer to the question you pose.
The other day I had occasion to correspond with Paul Cochrane (alluded
to in his posting in this thread) and particle. Here's some of what I
wrote them about Parrot::Distribution, two test files which use it
(t/perl/Parrot_Distribution.t and t/codingstd/perlcritic.t), and what I
think should be taken into consideration in refactoring all three files:
1. In Parrot::Distribution POD, clearly state P::D's purpose.
2. In P::D POD, identify P::D's inheritance tree. (particle spelled it
out on IRC once, but I didn't write it down.)
3. In comments on P::D source code, identify source of each method not
defined in the package itself. The ones I've identified are these:
file_exists_with_name
file_with_name
read
parent_path
directory_with_name
path
existing_directory_with_name
existing_file_with_name
delete
4. In P::D POD, explain rationale for singleton structure of object.
5. In P::D POD, under instance methods, most, if not all, of the
"Returns ..." statements are inaccurate or misleading. Do the methods
return "files" (presumably, strings containing filenames) or do they
return objects which are queryable with one or methods which in turn may
return filenames? The last "Return statement (under ops_source_files())
gives two different, not necessarily consistent descriptions of what the
preceding methods return.
6. print 'WARNING: ' . __FILE__ etc. We can use a module I submitted
as part of my recent ops2c.pl refactoring, Parrot::IO::Capture::Mini to
capture this statement during testing and test whether we got the
expected error message. (Jerry: Looked at those patches yet?)
7. Paul: As noted in previous email, the patterns you've got in
get_perl_exemption_regexp() are not Perl regexes but shell filename
expansion patterns. So they can't be used as Perl regexes in tests.
[Alluded to in Paul's posting to this thread.]
8. is_perl(): Is opening a file and reading its first line a valid way
of identifying whether a file is Perl or not? What if the first line
were an SVN Id tag? [Paul C. responded to this: Actually, if it is a
perl file it should have a shebang line which *has* to be the first line
of the file, hence the rationale. I'm pretty sure Will wrote the first
version of this. It used to be in the tests, but I moved it into P::D
so that more tests could make use of it.]
9. file_for_perl_module(): POD: What does it mean to return a 'file'?
Is it really a file *name* that is being returned?
10. docs_directory(), html_docs_directory(), delete_html_docs(): These
ethods serve a very different purpose from those that precede them.
Would they be better placed in a subclass of P::D?
11. gen_manifest_skip(): AFAICT, this is not a Parrot::Distribution
method but simply a subroutine found in the P::D package. We should not
mix functional and OO code in the same package. Let's find a better
place for it. [Paul C. concurred.]
12. t/perl/Parrot_Distribution.t: Some of the above comments apply to
this test file as well. One test failing; see (7) above.
13. t/codingstd/perlcritic.t: Weird options processing. What is the
rationale for this method of options processing? Why not use a
GetOptions instead? (The other build tools use that.) [Paul C. agreed
that GetOptions should be considered here.]
As you can tell, answering the above questions ultimately depends on an
accurate enumeration of all of Parrot::Distribution's current uses and,
distinct from how it is *currently* used, a specification of how we
think it *ought* to be used.
Since refactoring of Parrot::Distribution is something which is going to
affect the entire build-and-test process, I recommend that the
refactoring *not* proceed in the Parrot repository's trunk but in a
branch instead. I've been doing my refactoring of the build tools such
as ops2pm.pl in the 'buildtools' branch and only posting them as patch
submissions to trunk when I have a high degree of confidence in them.
If you would like to work on refactoring Parrot::Distribution in that
branch, please feel free to do so and let me know.
(In the email correspondence mentioned above, particle said he would
welcome an addition to the documentation which gives advise on how to
use Subversion branches in Parrot development. Yesterday I posted 4
files to the buildtools branch which do that; you can see them there:
lib/Parrot/Subversion/BranchManager.pm
tools/util/svn_new_branch.pl
tools/util/svn_merge.pl
docs/svn_branching.pod
The first three files are in good shape; I'm completing the fourth and
hope to finish it this weekend. When I do so, all four will be
submitted as patches to trunk.)
kid51
