Hi Keith. Thanks for your review. All items you called out which have been omitted from this email I accepted without comment.
new webrev vs main python gate: http://cr.opensolaris.org/~schwartz/091101.1/webrev/index.html new webrev vs old webrev: http://cr.opensolaris.org/~schwartz/091101.1/webrev-2-1-diff/index.html (look for the files which have my bug ID: 12466) Comments inline. On 11/04/09 11:02, Keith Mitchell wrote: > > install-tools/ManifestServ.py: > 76: Did you intend to put "if not path:" here? Yes. Changed. > > As a more general comment, this loop would be more appropriate if the > while clause evaluated path in some way, e.g., > path = sys.stdin.readline() > while path: > ... > <do_stuff_here> > ... > path = sys.stdin.readline() In general I agree. Here, though, I want to print a prompt before taking a line to evaluate. This means I would need to replicate the prompt and reading the line, once before the loop and once at the end of the loop. This IMO introduces more complication to the code than evaluation inside the while clause would take away. > > 78-79: As the main clause catches KeyboardInterrupt, why bother > catching anything here? To break out of the loop. > > > 108: Nit: This function doesn't return anything. Hmmm... I thought about this earlier, what to do here... So yes technically this method doesn't return anything... but it is a constructor and as such it aids in building an instance of _HelperDicts which is why I put that comment there. > > > 197-204: I don't see anything here that would warrant the try/except > block. OK. You are correct. items() doesn't throw StopIteration. next() does. Removed the try/except. > > 208: "if not match:", or combine this block with the previous for loop > using for/else syntax. Thanks. Changed to use "else" syntax. > > Based on 187 and 211-227, this code looks very fragile. At a minimum, > the constraint of module name = class name will force us to break PEP8 > naming conventions. Can you file a bug for looking into alternatives > to this usage scenario (assuming that the 2.4->2.6 name changes here > and elsewhere don't cause this code to break, forcing you to come up > with a solution now)? I documented the naming constraing better in the "new" method of HelperDicts. The class name being the same as the module name was to keep the manifest simpler. Coming from a Java background where class name = module name is not only common, but enforced by the language, I'm surprised that PEP8 is the opposite, but so it goes... Other than the naming convention and constraint, what else do you think needs to go into the bug report? > > 424-430: Could be "if method_ref not in deflt_setter_dicts.modules or > not in deflt.setter_dicts.methods:" Changed to: if ((method_ref not in deflt_setter_dicts.modules) or (method_ref not in deflt_setter_dicts.methods)): > > > 717: Change this to "!=" instead of "not ... ==" would be easier to read. The "not" applies to more than the "==". > > > 953-957: This change masks potentially useful traceback data. OK. Got rid of HelperDictsError and just throw an original ManifestProcError now. > > > 1126: "if errors:" OK > (Sidenote: If we raise an exception if any error occurs, why don't we > raise right away, to provide better context for where the error > occurred...) So one run will catch all the problems, instead of having a new problem show up each time a new run is made. > > 1127,1188: Change to "raise ManifestProcError(..." I'd rather not do this at this point. I'd have to change this everywhere and retest every error condition. > > 1265, 1316: Sidenote: These functions will mask underlying errors and > may make things more difficult to debug. TreeAccErrors show useful messages. Other (programming) exceptions will still be thrown. > > ENParser.py: > > Side comment: __STATE_TABLE might be better off as a dictionary, but > that's certainly out of scope ;) > > > 482-484: Could be replaced with "for curr_char in nodepath:" True, except that there is an error message which mentions the index inside the loop body. > > install_utils/ManifestRead.py: > General: In "except <SomeError>, err:" clauses, if err is never used, > please change to just "except <SomeError>:" ??? Didn't find any of these in this file. I'll keep an eye out for this in other files. > > > 194-202: Change to "except (socket.error, IndexError, ValueError):" or > "except StandardError:" Actually, this is one of the scenarios where a > bare 'except:' clause is acceptable, as you simply log that an error > occurred and re-raise. Consolidated the error types. Left them explicitly stated as I think it makes the code clearer about what is going on. > > install_utils/ManifestServ.py: > > 55: Please base this class off of StandardError. Yes, and all other exception classes as well. > > > 304-307: I think the previous syntax of re-raising the thread.error > was more appropriate. Masking it as a ManifestServError loses some of > the traceback information. True, but this is condition is due not to a programming error, but to a system condition. Why muddy the message with a traceback? > > 340-343: I think this should be in a 'finally' clause. You are right in that the socket close and unlink should be executed no matter what. Since exception handling is "pass", there is no need for a "finally" clause. I changed the flow so that these things always execute, however. Each now has their own try/except which swallows errors. > > 448: I think this should be "if not pre_request:" or left unchanged. OK. if not pre_request. > > 553: Here, and in the other location(s) where you make this note, I > think it's appropriate to add the in-line comment that disables it, > and reference http://www.logilab.org/ticket/8764 as the reason for > disabling temporarily. I added the inline comment to disable pylint at that location. I also added the reference; this seems like the best solution, however in the past we were not allowed to add bug IDs as code comments. This is not OUR bug though... Let's see if anyone protests... > > 590-604: Why are we now returning instead of re-raising? Because this method is called in a thread, and this is the highest stackframe in this thread. Raising here won't terminate the main thread but only dump a traceback. Seemed better to just print a useful message. I have embellished the messages to print the exception info. > > TreeAcc.py: > > 571-575: This change causes us to lose some traceback information. IMO, a good message will be more useful to an end user than traceback info. > > 812: This final return statement is unnecessary. OK > Sidenote: This function would be more appropriate if it returned > found_nodes, and the callers were updated to set found_nodes = > __search_node(...) Yeah... But I think I'll leave this alone. This routine is local to this module and I don't see a pressing need to change this at this point. > Starting on finalizer module... > > 232-235: Just don't catch theses, and modify 236 to "except > StandardError:" as the 'catch all' I want OSError and socket.error to be skipped. Did implement StandardError as catchall however. > 254-258,289-293: Ditto Ditto. > > 399: This seems to be a pretty significant change that removes the > ability to specify "python-module:function" as indicated by the > function docstring. If this is intended, please update the docstring; > if not, please revert. Oops. Yes. Docstring updated. > > 436-437, 439-440: Sidenote: If arg is always a string to begin with, > we could convert these to "shell_list.extend(<list>)" Arg isn't always a string. > > install_utils.py: > 255-265: Could be simplified to: > try: > int(arg, 0) > float(arg) > return True > except ValueError: > return False Nope. "0x123" will get caught on the float and return false. > > 366: At minimum, let's set this to "while True:" (though actually > evaluating a condition would be even better) Changing to while True, but leaving the rest alone. No time to investigate this and I didn't write this code. > > > 424: "if output.endswith('\n'):" I don't see what this buys us. Leaving alone. Thanks again for your review. Tag, you're it... Thanks, Jack > > Jack Schwartz wrote: >> Hi everyone. >> >> Here are 2.4 -> 2.6 and PEP8 changes for install_utils used by DC and >> other parts of the install mosaic. Please review. (Clay, are you >> available for this?) >> >> http://cr.opensolaris.org/~schwartz/091101.1/webrev/index.html >> >> All files achieve a 10 by pylint, with the following caveats: >> - usr/src/lib/install_utils/ManifestServ.py: --disable-msg=C0321 >> which thinks the try of try/except/finally is two statements on one line >> - TreeAcc.py: --disable-msg=E1103 since pylint thinks some datatypes >> are lists when they are not. >> These are documented in the code. >> >> I have tested all changed files: >> - tested parser files by executing all functions of TreeAcc module >> - tested many different nodepaths to exercise ENParser module >> - testing both good and error cases for validation and default setting >> - verified ManifestRead socket interfaces >> - verified finalizer called modules with args correctly >> >> There are many changed lines, but most are formatting for PEP8, so it >> shouldn't take too long... >> >> Changes other than formatting changes include: >> - condensing some nested (try/except - try/finally) into >> try/except/finally >> - changed reader in TreeAcc from xml.dom.ext.reader to xml.dom/minicom >> - wrote a tree walker in TreeAcc to replace the one which went away >> in 2.6 >> - got rid of Trace in install_utils >> - removed py function call handling (which never worked and gave 2.6 >> pylint errors) from finalizer >> >> Thanks, >> Jack >> >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
