Hi Drew, Thanks for the detailed feedback.
On 07/19/10 06:40 PM, Drew Fisher wrote: > Darren, > > Here's my code review. I only touched the .py files as my experience > with Makefiles involves stealing working Makefiles from others. :) I'm much the same I admit when Makefiles are involved... > > GENERAL NITS: > > - Make sure you run pylint and pep8 on the code. I've run pep8 and pylint a plenty, there are still some warnings from pylint, especially in the test directory, but most of these are to do with the length of the method names... > > - All of your CDDL headers need to delete the the second line with the "##" > characters: Done. > > - 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. Done. > > - unless you have compound conditionals in your if-statements, you can > remove > all the parentheses. Done. > > - don't use type(variable) is list/dict/obj, etc. Use isinstance Done. > > - remove all extra spaces around '(', ')', '[', ']', '{', and '}' > characters. Done. > > - tests for None can be done with > > if not variable: > > instead of > > if variable == None: > > and likewise for not None. I followed the PEP8 standard on this, and used "is None" and "is not None" as appropriate. > > ========================== > > data_object.py > > 67: remove the blank line Done > 83: change to: > if not LOGGER: Changed to "if LOGGER is not None" as per PEP8 > > 182, 186, 194: change these to use @property. This will allow you to > remove Done. > 190-192: commented code? Can this be removed? If not, it also needs > to be a > property as above Removed. > > 213: s/of/if Done. > > 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? Done, re-did the indenting, passes pep8 now. > 292-299: the logic is a bit clunky, imo. Why set a variable to evaluate > later, rather than just return the child object? > Adjusted as suggested, doing returns. > > 342-344: see my comment for line 259 Done. > > 384: remove extra blank line Done. > > 386-388: see my comment for line 259 Done. > > 443: TODO comment Removed. > > 449-450: combine these into one line > Done. > 458-459: combine these into one line Done. > > 483: remove extra line > Done. > 498-499: see my comment for line 259 > Done. > 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 Done. > above > 'if children' conditional? If not, I'm clearly confused by the code :) I updated the comment to explain a little, but the pydoc for this method also explains that if the specific children are specified, then the rest of the conditions are ignored, which is why this loops to delete specific children and then returns after all the children are handled. > > 554-566: the logic here matches the logic on lines 292-299. Can you > clean it > up the same way? I could, but I felt that this logic was more read-able than the alternative which would require repeating 2 lines of code for each alternative. The different between this an the lines 292-299 is that I want to continue if something is found while the previous one stopped after finding the first match. > > 569-571: see my comment for line 259 Done. > > 573: remove extra line Done. > > 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? Firstly, I prefer the explicit provision of the copy/deepcopy methods on the object itself - but that's just me I'm guessing ;) As for the reason I didn't use the __copy__() and __deepcopy__() implementations to achieve the same result, well I have a couple: 1) Specifically for deepcopy() there is an issue where it will traverse up the tree via the parent reference - if I was to change __deepcopy__() to not do this then it would have a knock-on effect of also doing the same for the children objects - which I DO want to be copied completely, including the parent reference. I can't see any easy way to ensure this is done by defining __deepcopy__(). 2) While copy is fairly simple - and it may be possible to do what I want with a redefinition of __copy__() it could be seen as inconsistent with deepcopy() if I omit it. 3) It expected that DataObject will be sub-classed, and if anyone creates an object that requires some complex copy mechanism themselves, then they will be probably overriding the __copy__() and __deepcopy__() methods - and if this happens we could end up with things mis-behaving without the implementor realizing. I will though take a closer look at this though since you're not the first to mention it. > > 689-696: convert to using @property as on lines 182, 186, etc. Done. > > 747-746: see my comment for line 259 Done. > > 756-758: see my comment for line 259 Done. > > ========================== > > data_object_cache.py > > 40: why is _LOGGER used over LOGGER as done in data_object.py? Actually, the one in data_object.py should be _LOGGER too - mainly because it's a module private variable what I don't want to expose. > > 77, 91: line is longer than 80 characters Fixed. > > 109-119: ignoring due to FIXME comment Yes, this code will be removed as soon as I get confirmation that the DC implementation is obtaining the DOC from the Engine rather than using this singleton mechanism. > > 121: remove spaces between * and ** characters Done. > > 153-162: switch to using @property as detailed above Done. > > 169-170: see my comment for data_object.py:259 Done. > > 226: add a space after the comma on the hasattr() call Done. > > 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. Pickle is being used as a way of serializing the contents of the Data Object Cache for snapshot and resume. We are aware of the problems with Pickle, and it has been mentioned in the design document that it's has these limitations. Shelve isn't all that different to Pickle, if anything it's exactly the same except that it stores the data in a database as opposed to a stream of data. I feel that Shelve is more than we currently require right now since it's not expected that there is any requirement to randomly access the contents. > > 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. Hmm, I did change to using class_obj as you suggested, but I just re-ran pylint and it's complaining that I should be using 'cls' for my class methods - so I guess I'll be reverting that part... > > 322-326: Can you use dict.setdefault() here? > > _CACHE_CLASS_REGISTER.setdefault(priority, []).append(klass_ref) > > one liner! :) I like this, didn't realise there was such a convenience method on a dictionary. > > 339, 346, 361 : ugly 'klass' variable. Changed all 'klass' to class_obj. > > 371: change to: > def import_from_manifest_xml(self, dom_root_node, volatile=False): Done. > > 371: add docstring. Done. > > > ========================== > > > data_object_dict.py > > GENERAL: There are a LOT of extra spaces around parens in this file. pep8 > will help you find 'em all. Done. > > GENERAL: There are a few lines of if type(variable) == data_type. Change > these to isinstance() tests. Done. > > 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? I changed to using data_dict instead of dictionary - it's more in line with the definition of the class anyway - just means I need to update my design document slightly... > > 121: why are you using printf-style coding here? why not just do > sub_element.text = str(self.dictionary[k]) > ? Historical, removed it. > > ========================== > > unit tests: > > GENERAL: > > - fix imports as above Done. > - remove extraneous spacing around parens and ":"s Done. > - add docstrings to some of the more populated methods for anything "tricky" Not done yet, will revisit. > - change away from 'klass' and 'cls' named variables as explained above Done. > - remove the singleton 'pass' lines in all of the tests. i.e. line 274 of > test_data_object_fetching.py > Done. > ========================== > > 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): > ? Done. > > 107, 114: line is too long. can you reduce it or create an error > string variable? I've reformatted it. > > ========================== > > 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. Done. > > 137, 143: line is too long, see 119 for example cleanup > Done. > 152-157: use if os.path.exists(self.temp_file): instead of try/except > 159: same comment > Done. > 300: add a space after the comma > Done. > 365: remove line Done. > > ========================== > > test_data_object_cache_xml_support.py > > line 82, 235: add spaces around the '=' character > Done. > ========================== > > test_data_object_deletion.py > > 146-147, 211-213: remove spaces around parens > Done. > ========================== > > test_data_object_dict.py > > 35, 40, 46, 51, 56: are these pass lines needed (honest question, I don't > actually know) They were needed when I first defined the classes since there wasn't much in them, but once I put in the entries to change the class variables then they became superfluous. > > 201, 251: line is too long Done. > > ========================== > > test_data_object_fetching.py > > 99, 117, 123: line is too long. see comment above Done. > > ========================== > > test_data_object_insertion.py > > 129: remove this line Done. > > ========================== > > test_data_object_utility.py > > 99, 117, 123: line is too long. see comment above Done. > > 107-108: add 4 spaces to the continuation line to be consistent. Done. > > > ========================== > > test_data_object_xml_support.py > > 133, 151, 157: line is too long. see comment above Done. > > 141-142: add 4 spaces to the continuation line to be consistent. Done. > > ========================== > > Whew! Yeah, a marathon review :) Thanks again, Darren. _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
