Hi Dermot, Thanks for taking a look, comments below...
On 09/ 7/10 07:19 PM, Dermot McCluskey wrote: > Hi Darren, > > This is my first pass through the main code. I have not looked at the > unit tests yet. > > I did not find any serious errors in the code. These comments are > mostly typos, nits, and suggested changes to comments, plus a few > queries about the code. > > - Dermot > > > usr/src/lib/install_common/* > ==================== > What is the "install_common" directory for? Is it being used > anywhere? The purpose is to have a home for the __init__.py that is used in the new solaris_install python package - without it, it's not seen as a package. > > > usr/src/lib/install_doc/data_object/__init__.py > ================================= > 43 class DataObjectException(Exception): > 48 class ObjectNotFound(DataObjectException): > I believe the PEP8 guide for exception names is that they have > the suffix "Error", so DataObjectError and ObjectNotFoundError > would be better here: http://www.python.org/dev/peps/pep-0008/ Didn't know this one... Changed. > > 66 Every object the is stored in the Data Object Cache is required > typo: "the" -> "that" > Done. > 67 to sub-class the DataObject class, and implemement the required > typo: "implemement" -> "implement" > Done. > 80 - the XML import/export mechamism > typo: "mechamism" -> "mechanism" > also, should add another comment here, eg: > - path-based searching Done. > > 126 provided their own XML representation from their to_xml() > typo: "provided" -> "provide" Done. > > 130 Should there be a desire that a class would handle the XML > 131 for it's children, the this is possible to do by setting the > 132 object variable "generates_xml_for_children" to True, and > awkward wording. Suggest: "If a sub-class of this class wishes to > handle the XML for its children as well as for itself, then set the > instance variable "generates_xml_for_children" to True and ..." > or something better... Done. > > 135 The XML returned here should be as close as possible to > 136 it's equivalent in the AI/DC Manifest Schema. > Given our recent conversation, do you still want to leave this > comment in? Yes, for sure - the XML generated by a to_xml() method should match the XML in the schema where at all possible - not necessarily it's location in the overall manifest, but certainly it should generate XML that matches it's specific snippet of XML to make the restructuring of the manifest as simple as possible. This is perfectly logical and symmetric given that it reads in the XML from a manifest, then it should generate almost exactly the same XML in it's to_xml()... > > 144 This method is used, when importing XML, to quickly determine if an > 145 object implementation is able to take an XML Element, and convert it > Suggest: "an object implementation" -> "a class" Done - also changed 'take' to 'process' > > 149 method should be implemented to quickly examine it to decided if > typo: "decided" -> "decide" Done. > > 162 object. > Suggest: "object" -> "class" Done. > > 138 return None > 164 return False > 194 return None > Does it make sense to have the declaration of abstract methods return > anything? Would "pass" be more appropriate? Using 'pass' certainly makes sense in some way, but I feel it's useful to return suitable values should someone desire to call up to a super-class method implementation for any reason. > > 198 # These are accessor and mutator methods for simple class properties. > There are no mutator methods here, only accessors. Removed ref to mutators. > (See comment at end about fget/fset-style properties.) Don't need to use that style if read-only, that's what the @property decorator is meant for. > > 236 You may specify one, or both, of the following to narrow the list of > Either change: "one , or both, of" -> "one or more of" > or add a sentence before "max_count" to distinguish it from the other > criteria Went for the 'or more' option since they all can be combined to limit the find - including just max_count. > > 245 max_count - Limit the number of children to be searched for. > 344 max_count - Limit the number of children to be searched for. > Does this limit the number of objects "searched" or "returned"? > I think it would be clearer to say "Limit the number of objects returned." It actually "limits the number of children to be searched for" - on finding that number it stops searching. To say that it "limits the number of objects returned" doesn't imply that it stops searching after finding that number... So, as a compromise, I changed it to read: max_count - Limit the number of children returned, searching stops on reaching this number of matches. > > 254 modifying the will not effect the internal list of children, but > typo: "the" -> "the list" (or "it") Done. > > 263 # If no class_type given, assume DataObjectBase, otherwise > 264 # get_descentants will return error. > get_descendants() defaults class_type to DataObjectBase, so this > should not be needed. Not totally accurate - get_descendants() requires that at least one of name or class_type is specified - this code here ensure that at a minimum the class_type is specified by a call to get_children() where it's optional. > > 271 # Define children property, don't use @property since get_children is > 272 # itself part of the exposed API. > 273 children = property(get_children) > Move to c. line 217? Can't - will generate a compilation error if I do since get_children is not defined there yet... > > 278 This method returns a the first child object that match > typo: "a " -> ""; "match" -> "matches" Changed to: This method returns a reference to the first child object that matches > > 281 By default, of no criteria is specified, a reference to the first > typo: "of" -> "if" Done. > > 318: > Should also add a comment to get_first_child() and get_children() > stating that depth-first search is used. I don't agree - it's not relevant to these since they are limited to a max_depth of 1 - which means direct children. A depth-first search is only relevant if you got deeper than 1 level. > > 341 such a search in a large tree structure. A value of 0, > 342 or less, means the depth should not be limited. > According to the code, values of less than 0 causes an exception > to be raised, so "or less" should be deleted here. Changed to be a "value of 0, or None, ..." which is more correct. > > 507 element = etree.Element("dummy") > As discussed previously, do you want to change this to, eg "root"? Yes, I think that makes sense - root definitely is more accurate. Changed, and changed comment to match. > > 598 '''Fetches element of DataObject tree structure using a path. > & > 636 This method returns a list of matches to the criteria. > - If your search specifies an attribute (/name.attr) is the return value > a list of objects or a list of attribute values? I wasn't sure from the > comments which is returned. If you specify an attribute, it will return a list of matches attribute values - if you don't it's a list of objects. I thought it obvious from the text, but I'm biased, so I changed to read: This method returns a list of matches to the criteria. The list will contain objects if no attribute is specified, otherwise it will contain the values of the attribute from each matched object. > - If it's a list of attribute values, then is it possible to search for an > object that has a particular attribute (ie only return the object if the > particular attribute exists on it)? Not in the sense that I think you're asking. If you specify an attribute that doesn't exist (or starts with _) then it will result in an AttributeError... > - if your constraints don't match anything, an error is raised. If your > constraints *do* match something, but the specified attribute doesn't > exist, then you just get an empty list. Is that correct? Should you not > also raise an error for this? You don't get an empty list, you will get AttributeError's raised - this is generated by the call to 'getattr()'... Added a comment to mention this. > > 607 @type => A fully-qualified class string for which is mapped > typo: delete "for" Done. > > 625 breaks matches a child with the name "*/a b". > typo: delete "breaks" ? Yep - remnant of some older text. > > 723 @staticmethod > 769 @staticmethod > Is there any benefit to having these as static methods within the class, > rather than just module functions, defined outside the class? The main benefit I see is that having it in the class makes it more obvious of it's limited use - to put it in the module might imply other uses of it, which there aren't. I feel it's better to avoid clutter in a name-space if it's not needed. > > 799 '''A variant of DataObject which disallows insertion and deletion. > 800 > 801 Primarily meant to be used for any object which is to be considered a > 802 leaf-node in the tree, or where an object wants to limit the usage > 803 of children. > I don't understand these comments, as this class *does* have insert > and delete methods? Oops - that's an old comment that reflected the old class structure! Changed it to read as : class DataObject(DataObjectBase): '''A variant of DataObjectBase which allows insertion and deletion. This is the class that most people will sub-class when creating an object for insertion in to the Data Object Cache. ''' > > 805 Known usages are DataObjectDict, and DataObjectCache. > Both of these are sub-classes of DataObjectBase, not DataObject. > Did you mean DataObjectCacheChild? Removed this comment. > > 829 This is raised if new_children is not a DataObject instance, or is > 830 a list containing objects that are not DataObject instances. > "DataObject" -> "DataObjectBase" Done. > > 922 if not found_children: > line should be deleted? Deleted it. > > > > > usr/src/lib/install_doc/data_object/cache.py > =============================== > 39 _CACHE_CLASS_REGISTRY = dict() > Given that DataObjectCache is no longer a singleton, am I correct in saying > that all DOC instances must share the same registry of classes? > (Probably never > a problem - I just want to clarify.) Given the nature of the registration of classes - i.e. they are done when there isn't an instance of the DataObjectCache yet this still seems to be the correct location. BUT - if people feel that it's an issue, maybe it could be moved into being a class attribute (as opposed to an instance attribute). > > 107 Sub-classes DataObjectNoChildManipulation to disable add/remove > Doesn't exist? > Has this class been renamed since this comment was written? Yes, as mentioned in the e-mail I changed the class structure, it should read: Sub-classes DataObjectBase to ensure that its not possible to add or remove the "persistent" and "volatile" children. > > 123 # Add children using DataObject implementation of insert_children, > 124 # since we're overriding implementations in DataObjectCache itself to > 125 # disallow this. > Does this comment still apply to this block of code? If so, I don't > understand > it. Old comment, removed. > > 352 By default, because 'volatile' is False, the XML will be imported > 353 into the DataObjectCache.persistent sub-tree. > Would it not make more sense to have volatile default to True? No, the preference was meant to be that by default everything went into the persistent sub-tree - and that the only one that didn't want this would be the DC implementation since it was the only want where it was expected that a modified manifest should take preference at all times on resume. Do you think this is wrong then? > > usr/src/lib/install_doc/data_object/data_dict.py > ================================= > 166 # Attributes accessors > There are both accessor and mutators here Change the comment. > > btw, should we use the fget/fset convention that Keith (I think?) showed > us for > properties, eg: > > def currboot(): > def fget(self): > return self._currboot > def fset(self, val): > if not isinstance(val, bool): > raise TypeError("currboot must be boolean") > self._currboot = val > doc = """True if this Disk is current boot disk""" > return locals() > currboot = property(**currboot()) Hmm, I don't remember this one, but I did change the DataObjectDict to use it. Thanks, Darren. _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
