Darren,
Here's my code review. I only touched the .py files as my experience
with Makefiles involves stealing working Makefiles from others. :)
GENERAL NITS:
- Make sure you run pylint and pep8 on the code.
- All of your CDDL headers need to delete the the second line with the "##"
characters:
#
##
# CDDL HEADER START
#
Needs to become:
#
# CDDL HEADER START
#
- All of your imports needs to be in the following format:
(from the pep8 guide here: http://www.python.org/dev/peps/pep-0008/ )
1. standard library imports
2. related third party imports
3. local application/library specific imports
You should put a blank line between each group of imports.
All in alphabetical order.
(example from data_object.py)
import copy
from abc import ABCMeta, abstractmethod
from lxml import etree
import logging
Needs to become
import copy
import logging
from abc import ABCMeta, abstractmethod
from lxml import etree
- unless you have compound conditionals in your if-statements, you can
remove
all the parentheses.
if (class_type == None and name == child.name):
becomes
if class_type == None and name == child.name:
(before other issues, as addressed below)
- don't use type(variable) is list/dict/obj, etc. Use isinstance
- remove all extra spaces around '(', ')', '[', ']', '{', and '}'
characters.
pep8.py will catch this for you.
- tests for None can be done with
if not variable:
instead of
if variable == None:
and likewise for not None.
==========================
data_object.py
67: remove the blank line
83: change to:
if not LOGGER:
182, 186, 194: change these to use @property. This will allow you to
remove
lines 393-397
@property
def name(self):
return self._name
@property
def parent(self):
return self._parent
@property
def has_children(self):
return (len(self._children) > 0)
If you need setters, you'll have to define a real property as explained
http://docs.python.org/library/functions.html#property
190-192: commented code? Can this be removed? If not, it also needs
to be a
property as above
213: s/of/if
259: add a space between the '%' and '\' character. This will cause
the line
to go to 80 characters, so you'll have to do a little reordering of the
entire
message. Perhaps write the string out first and then just raise the
exception
with the string variable?
i.e.
if not found_children:
s = "No matching objects found: name = '%s' and class_type =
%s" % \
(str(name), str(class_type)))
raise ObjectNotFound(s)
292-299: the logic is a bit clunky, imo. Why set a variable to evaluate
later, rather than just return the child object?
for child in self._children:
# Look for matches to criteria
if not class_type and name == child.name:
return child
elif not name and isinstance(child, class_type):
return child
elif name == child.name and isinstance(child, class_type):
return child
342-344: see my comment for line 259
384: remove extra blank line
386-388: see my comment for line 259
443: TODO comment
449-450: combine these into one line
458-459: combine these into one line
483: remove extra line
498-499: see my comment for line 259
543-545: move this conditional to the top so that singletons are handled
quicker. OR, this seems to be better suited as an 'else' clause to the
above
'if children' conditional? If not, I'm clearly confused by the code :)
554-566: the logic here matches the logic on lines 292-299. Can you
clean it
up the same way?
569-571: see my comment for line 259
573: remove extra line
591 and 606: Why are you defining copy and deepcopy when you're import
copy
above? Is it strictly to handle the removal of the parent?
If you need to do this, can you provide comments in the docstring as to
exactly
why copy.copy() or copy.deepcopy() can't be used alone?
689-696: convert to using @property as on lines 182, 186, etc.
747-746: see my comment for line 259
756-758: see my comment for line 259
==========================
data_object_cache.py
40: why is _LOGGER used over LOGGER as done in data_object.py?
77, 91: line is longer than 80 characters
109-119: ignoring due to FIXME comment
121: remove spaces between * and ** characters
153-162: switch to using @property as detailed above
169-170: see my comment for data_object.py:259
226: add a space after the comma on the hasattr() call
233: Shawn Walker already commented on using pickle. I'm unsure of what's
being pickled and why, but you might want to look at shelve:
http://docs.python.org/library/shelve.html to see if this is a better
solution
for you
280: pickle comment as above.
293: change to
def register_class(cls, klass, priority=50):
293: I -strongly- dislike the name of the 'klass' variable. Can you change
this to class_obj or something that's a little more readable? This goes for
anything else staring with 'klass' for the entire file. I'm also not a
fan of
variables named 'cls' for the same reason.
322-326: Can you use dict.setdefault() here?
_CACHE_CLASS_REGISTER.setdefault(priority, []).append(klass_ref)
one liner! :)
339, 346, 361 : ugly 'klass' variable.
371: change to:
def import_from_manifest_xml(self, dom_root_node, volatile=False):
371: add docstring.
==========================
data_object_dict.py
GENERAL: There are a LOT of extra spaces around parens in this file. pep8
will help you find 'em all.
GENERAL: There are a few lines of if type(variable) == data_type. Change
these to isinstance() tests.
77: I'm not a fan of naming variables things like 'dictionary'. a) it's a
long word to type (laziness checking in!) and b) it's a data type, but
unlike
list and tuple, you create dictionaries via a different function call.
(list(), tuple() and dict()). This is more of a personal nit of mine and
clearly not code impacting. Is there any way this can be changed around so
that it doesn't appear to be manipulating an entire data type when you're
clearly using it as just a variable?
121: why are you using printf-style coding here? why not just do
sub_element.text = str(self.dictionary[k])
?
==========================
unit tests:
GENERAL:
- fix imports as above
- remove extraneous spacing around parens and ":"s
- add docstrings to some of the more populated methods for anything "tricky"
- change away from 'klass' and 'cls' named variables as explained above
- remove the singleton 'pass' lines in all of the tests. i.e. line 274 of
test_data_object_fetching.py
==========================
test_data_object_cache_registration.py
104: line is too long. can you change the function to
def test_doc_registration_simple_data_object_prio_minus_1(self):
?
107, 114: line is too long. can you reduce it or create an error
string variable?
==========================
test_data_object_cache_snapshots.py
119: line is too long. change to:
self.child_2_1_1.insert_children([self.child_2_1_1_1,
self.child_2_1_1_2])
OR
self.child_2_1_1.insert_children([self.child_2_1_1_1,
self.child_2_1_1_2])
since you seem to carry the 4 space indent on continuation lines in
other test files. Simply be consistant.
137, 143: line is too long, see 119 for example cleanup
152-157: use if os.path.exists(self.temp_file): instead of try/except
159: same comment
300: add a space after the comma
365: remove line
==========================
test_data_object_cache_xml_support.py
line 82, 235: add spaces around the '=' character
==========================
test_data_object_deletion.py
146-147, 211-213: remove spaces around parens
==========================
test_data_object_dict.py
35, 40, 46, 51, 56: are these pass lines needed (honest question, I don't
actually know)
201, 251: line is too long
==========================
test_data_object_fetching.py
99, 117, 123: line is too long. see comment above
==========================
test_data_object_insertion.py
129: remove this line
==========================
test_data_object_utility.py
99, 117, 123: line is too long. see comment above
107-108: add 4 spaces to the continuation line to be consistant.
==========================
test_data_object_xml_support.py
133, 151, 157: line is too long. see comment above
141-142: add 4 spaces to the continuation line to be consistant.
==========================
Whew!
-Drew
On 07/19/10 07:31, Darren Kenny wrote:
A gentle reminder ;)
Also, I've just updated the webrev slightly to include packaging and to add
data_object_cache to the file:
/usr/lib/python2.6/vendor-packages/osol_install/__init__.py
Thanks,
Darren.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
