Hi Matt.
Thanks for working with me on this and your explanation. All looks good
to me now.
Jack
On 12/17/10 08:32 AM, Matt Keenan wrote:
On 12/16/10 04:52 PM, Jack Schwartz wrote:
Hi Matt.
On 12/16/10 02:00 AM, Matt Keenan wrote:
Jack,
Thanks for the review, comments below :
On 12/15/10 07:43 PM, Jack Schwartz wrote:
Hi Matt.
Thanks for doing this work. Here are my comments:
parser.py:
106-107: I would remove comment about DerivedManifest using this. IMHO
users of a method shouldn't be listed in the method's comments, as the
comment can sneakily go out of date.
Will remove any reference to DerivedManifest.
Consider putting 393-398 under the if of 390. No need to continuously
check that the manifest exists and is a file after doing it once.
Actually it's needed where it is. These checks used to be performed in
__init__() when the manifest argument was required.
However as they are not being done in the __init__() anymore they are
required here, to check for __init__() argument as well as for doc
retrieved manifest.
No problem here.
The check for _manifest is None should always be done,
Here's where I am not clear. As I see it, __init__ gets called once and
may set _manifest. Then the manifest property is accessed invoking the
code in question here, which can set it from the DOC.
If manifest is set via __init__() then the DOC is never accessed.
Is there a way for _manifest to get changed between manifest property
accesses, so that _manifest has to be checked each time? I assumed not,
based on the original code (where init set things up and that was it).
If that is the case, then the code from 391 - 398 need run only once (as
it used to in __init__.
_manifest can only be set in two places and for each parser
instantiation.
If set via __init__() then DOC is never accessed, as _manifest will
not be None.
If not set via __init__(), then DOC is accessed to determine manifest
to parse.
Either of these methods of setting the manifest will only ever occur
Once.
The check to ensure _manifest is Not None can only be performed in the
get_manifest() method.
However the os.path.isfile() could be called in two places, either in
__init__() if manifest argument is not None, or inside the if
statement once the DOC has been read.
To avoid having the same checks in both places I simply placed them at
the end of the get_manifest() method, agreed they will get called
everytime _manifest is accessed, but I don't think that's an issue as
it's only accessed inside the _load_manifest private parser method,
and will not cause any undue inefficiency. If you feel strongly enough
I can split them back out again.
cheers
Matt
however the check for os.path.isfile(), if moved under the if as you
recommend would have to be added back to the __init__() if the
manifest argument was not None.
I don't understand this either. __init__ gets called once, and the
manifest property code can change _manifest if it wants to. If _manifest
changes, the manifest property code can call os.path.isfile() to
verify it.
So, I thought that 391-398 needed to execute only once to set up
_manifest from the DOC if not already set up in __init__, and once
things were set up, they stayed in place, which is why I made my
original suggestion.
Thanks,
Jack
As it is having it in one place to be makes more sense, and it's very
little overhead.
428: I think can_handle() has to return False instead of None here.
While None may work, I see the DOC calls it and expects a boolean.
(lib/install_doc/data_object/cache.py line 349). That same file also
implements a can_handle() method which does the same as yours, and
returns False for the same reason
that ManifestParseData's returns None:
def can_handle(cls, xml_node):
'''The DataObjectCache class doesn't import any XML itself.'''
return False
Will change to return False.
The other files look fine.
thanks
Matt
Thanks,
Jack
On 12/15/10 09:32 AM, Matt Keenan wrote:
Hi,
I'd like a couple of pair's of eyes to scan over a fix for bug :
7004864 Read profile to be parsed from DOC if not specified via
arguments
http://monaco.sfbay.sun.com/detail.jsf?cr=7004864
Webrev ;
http://cr.opensolaris.org/~mattman/bug-7004864/
This request modifies Manifest Parser so that a manifest being passed
at initialization is not a hard requirement. If not passed it will be
read from a location in the Data object Cache. This functionality is
being added to support Derived Manifests.
cheers
Matt
_______________________________________________
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
