Looks good to me. Thanks,
Darren. On 12/16/10 10: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. > > The check for _manifest is None should always be done, 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. > > 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 _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
