On 10/12/10 07:26 AM, Dermot McCluskey wrote:
Hi Karen,
Many thanks for your review.
An updated webrev which includes fixes for the issues you raised is at:
http://cr.opensolaris.org/~dermot/webrev_mp_mw_04/
Hi Dermot,
The updated webrev seemed to have addressed my comments.
I have a few more comments inline below on the original comments.
Responses below.
* line 83, do we need to validate and make sure that "dtd_file" is valid
before making this call? If it is assumed that the dtd_file is
valid, I think
it would be a good idea to say so in the parameters section of the
comment.
This should not be necessary. The call to etree.DTD(dtd_file) raises
DTDParseError
for both syntax errors in the file or if the file is not present. If
the file is not present
the returned ManifestError from validate_manifest() will look like:
ManifestError: Unable to parse DTD file
[blah/lib/install_manifest/test/manifests/dc.dtd]: : DTDParseError
- failed to load external entity
"blah/lib/install_manifest/test/manifests/dc.dtd"
which I think is a fairly descriptive message?
Sounds good enough.
* line 142: Should the directory path for the "manifest" argument be
check to ensure
that it is valid?
It's much easier to try to write to the manifest file later on and return
an error if that fails, rather than check up front if it would be
possible to
write it.
I think it would be being overly defensive to fail in the initializer
method if a file to be created moments later cannot be created at that
time. Unless there's a particular reason you think this check would
be useful?
Actually, I was not thinking about creating the file. I was thinking
about the directory
that will be containing the file. If I specify something like
/a/b/c/filename
and only /a/b exists but /a/b/c doesn't. There's no way we can create
the file.
So, I thought we should check it before hand instead of doing all the
computation,
and then, failing at the end. But I can also see why you don't want to
check it
in the beginning since it will fail eventually anyway. I am OK either way.
usr/src/lib/install_manifest/test/dc_*.xml:
* These are all DC manifests that are modified slightly for testing.
Instead of keeping these, I wonder whether it makes sense to get a
copy of
the most up to date DC manifest from the DC directory, and then,
generate
a file inserting/deleting things as appropriate for the tests. This
way,
we don't need to worry about keeping these files up-to-date as the DC
manifests
evolve.
It's only a coincidence that these files are based off the DC manifests.
These files are for unit testing very specific XML and DTD features in
MP/MW
and so it's best to keep them separate from the "real" DC files - it
would add
unnecessary complication if the unit test input files were in some way
linked
to the real DC files.
There is no need to keep these files in-sync with the DC files.
Thanks for the explanation. I thought they are related to the DC files
somehow
because their names are dc_*. Perhaps giving them all a more generic
name will
cause less confusion down the road?
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
