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?
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/
66 Every object the is stored in the Data Object Cache is required
typo: "the" -> "that"
67 to sub-class the DataObject class, and implemement the required
typo: "implemement" -> "implement"
80 - the XML import/export mechamism
typo: "mechamism" -> "mechanism"
also, should add another comment here, eg:
- path-based searching
126 provided their own XML representation from their to_xml()
typo: "provided" -> "provide"
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...
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?
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"
149 method should be implemented to quickly examine it to decided if
typo: "decided" -> "decide"
162 object.
Suggest: "object" -> "class"
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?
198 # These are accessor and mutator methods for simple class properties.
There are no mutator methods here, only accessors.
(See comment at end about fget/fset-style properties.)
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
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."
254 modifying the will not effect the internal list of children, but
typo: "the" -> "the list" (or "it")
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.
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?
278 This method returns a the first child object that match
typo: "a " -> ""; "match" -> "matches"
281 By default, of no criteria is specified, a reference to the first
typo: "of" -> "if"
318:
Should also add a comment to get_first_child() and get_children()
stating that depth-first search is used.
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.
507 element = etree.Element("dummy")
As discussed previously, do you want to change this to, eg "root"?
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 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)?
- 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?
607 @type => A fully-qualified class string for which is mapped
typo: delete "for"
625 breaks matches a child with the name "*/a b".
typo: delete "breaks" ?
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?
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?
805 Known usages are DataObjectDict, and DataObjectCache.
Both of these are sub-classes of DataObjectBase, not DataObject.
Did you mean DataObjectCacheChild?
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"
922 if not found_children:
line should be deleted?
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.)
107 Sub-classes DataObjectNoChildManipulation to disable add/remove
Doesn't exist?
Has this class been renamed since this comment was written?
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.
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?
usr/src/lib/install_doc/data_object/data_dict.py
=================================
166 # Attributes accessors
There are both accessor and mutators here
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())
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
