Hi Dermot,
First comment - the link given points to the wiki "preview" page which
requires edit privileges. The correct link would be:
http://hub.opensolaris.org/bin/view/Project+caiman/OpenSolarisInstaller+-+ManifestParser+and+ManifestWriter
My comments are below. Overall, the design looks good; I have a few
questions about the behavior of the interfaces, as well as the
exceptions exposed. (Thanks for calling out the differences from the
prior ManifestParser - that was interesting and a valuable point of
comparison)
3.4.1.1: The behavior and defaults for validate_from_docinfo leaves
something to be desired in the scenario of the XML not having a DOCINFO
section with a listed DTD. Better default behavior could be achieved by
setting the default to None, and then, "if validate_from_docinfo is
None" AND the supplied XML file has a given DTD, perform the validation.
If the caller explicitly wants validation from docinfo, they pass in
True (in which case, absence of the DOCINFO/DTD data would be an error,
and if they explicitly don't want validation, then pass in False.
page 13 - execute/dry_run behavior: I think a dry_run for ManifestParser
or ManifestWriter would still add items to the DOC. Consider for
example, a full dry-run of AI - the subsequent checkpoints would need
the data to perform their dry_run actions.
page 13: execute/raises: For an application attempting to recover from
such an error, how would they differentiate between the different error
cases and inform the user? If the manifest doesn't validate, how can the
app inform the user exactly where in the XML file the parsing failed? I
think ManifestParser needs a very solid, usable method for informing the
user of how to fix an error in their manifests, which means the errors
raised need to be readily understandable by the application and provide
plenty of data, so I think this section should be expanded. For example,
right now, depending on the error one has in an AI or DC manifest, it's
possible to end up with a generic, and useless "could not parse
manifest" error with no additional data - that doesn't really help an
end-user figure out that they had a typo on line 78 and forgot to close
the "<pkg_repo_addl_authority" tag.
In particular, subclasses of ManifestError that group the errors into
different cases would be valuable. And for some of those cases, the
error may benefit from providing more than simply string data. (e.g.,
line number, etc.)
An indication of how the ManifestErrors wrap around or enhance the
errors raised by lxml would be valuable as well.
page 14: What factors, if any, would affect the value returned from
get_progress_estimate() for this checkpoint? For example, transfer/IPS
depends on the number and size of packages being installed - are there
any similar factors at play here, or is it expected to be static (I'm
mostly just curious - I can't think of anything off-hand).
page 16: ManifestWriter/execute/dry_run behavior: For the
ManifestWriter, on the one hand it could be argued that a dry_run could
still benefit from presenting the user with an output manifest (perhaps
just on stdout?). Depending on how a user or developer running an app
specifies dry_run and an output file, it may be best to just always dump
to XML.
page 16: execute/raises: Again, more detail would be valuable, although
the ManifestWriter is probably better protected from end-user
errors/typos then the parser so this is perhaps less critical.
Thanks,
Keith
On 06/17/10 06:02 AM, Dermot McCluskey wrote:
Hi,
I hope there is still some enthusiasm left for reviewing design specs?
I'd like to request a review for my design for ManifestParser and
ManifestWriter.
The latest version of the document can be found here:
http://hub.opensolaris.org/bin/preview/Project+caiman/OpenSolarisInstaller+-+ManifestParser+and+ManifestWriter
As this component is slightly less complex than others that have
recently been
reviewed, I'd like to request that comments be sent by Friday June
25th. If this
is not practical, I will extend this date.
Please let me know if you intend to review this so I can be sure I
have enough
reviewers. I would particularly appreciate comments from some of the
following:
Jack, Alok, Karen, Darren, Sarah.
Thanks,
- Dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
