Hi Jack, Everything looks good. I have a few followup comments, below, but they're mostly nits and explanations of my thought processes.
I know I've been rather nitpicky with this and other Python CRs. I can't seem to help myself... small, gritty details for some reason catch my eye. Thanks for your patience! - Keith Jack Schwartz wrote: > 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: > >> >> 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. Alternate solutions include initializing "path = True" instead, for example. But that sort of change would require enough reworking of the loop that it's not worth the time. It's simply a suggestion for future thought. >> >> 78-79: As the main clause catches KeyboardInterrupt, why bother >> catching anything here? > To break out of the loop. KeyboardInterrupt will break out of the loop for you, and the main clause will catch it. However, after consideration, I think catching it here is also appropriate, such that the function works as a 'standalone' part of the module. >> >> 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. Perhaps saying "Results in:" instead of "Returns:" would be slightly more accurate? >> >> 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? That should be sufficient information for now. >> >> 717: Change this to "!=" instead of "not ... ==" would be easier to >> read. > The "not" applies to more than the "==". Ooops, 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. Oh, I see now. With all the 'print' statements, deferring allows all errors to be processed and printed. >> >> 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. > Fair enough. Keep in mind this is purely a syntax change: raise ValueError, "Description" is identical to raise ValueError("Description") The second format is the 'preferred' syntax, and should be used for new code (although the argument for 'consistency' within install would be enough to convince me to write new code with the old syntax). > >> 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. Understood. >> >> ENParser.py: >> > >> >> 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. Ah, I see the error message now. In that case, using: "for curr_char_idx, curr_char in enumerate(nodepath):" ...would allow you to ditch lines 483 and 580 (in the new webrev). (But feel free to ignore this as it's by no means required) >> >> 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. I might have had a case of blurry eyes. I've been looking through a lot of Python lately. >> >> 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? You're correct. I was getting overzealous. >> >> 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. That works for me. >> >> 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. Understood. Thanks for the explanation. > >> >> 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. Agreed. As mentioned above, I was getting overzealous. Sorry for the white noise. > > 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. Sorry for being unclear; however, the change you made is the one I was trying to suggest. >> >> 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. Well, I was hoping! >> >> 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. You're correct on that count, 2 separate try/except blocks are needed. However, the "dummy =" portion can be removed. >> >> 424: "if output.endswith('\n'):" > I don't see what this buys us. Leaving alone. It buys us PEP 8 compliance: - Use ''.startswith() and ''.endswith() instead of string slicing to check for prefixes or suffixes. startswith() and endswith() are cleaner and less error prone. For example: Yes: if foo.startswith('bar'): No: if foo[:3] == 'bar': > > Thanks again for your review. Tag, you're it... > > Thanks, > Jack > >
