HI Keith. Thanks for your second reply.
Keith Mitchell wrote: > Hi Jack, > > Everything looks good. I have a few followup comments, below, but > they're mostly nits and explanations of my thought processes. Phew! (Just kidding...) > > 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. It's taken some time, but I've also learned a lot along the way, so it's been useful. More comments below. I've "snipped out" the stuff I have no comment on. > > 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: >>> install-tools/ManifestServ.py: >>> 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? OK. I suppose saying anything here adds little value. It is obvious that it initializes an instance of its class; that's what a constructor is for. I'll just delete it for all of my __init__ methods, unless there is something useful to say in them. > >>> >>> 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. bug 12554 explore renaming of default and validation helper method modules and classes filed > >>> >>> 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. Leaving the "dummy =" in makes the code clearer IMO. > >>> >>> 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': Changed. New delta webrev: http://cr.opensolaris.org/~schwartz/091101.1/webrev-3-2-diff/index.html Main webrev vs python26 gate updated: http://cr.opensolaris.org/~schwartz/091101.1/webrev/index.html Thanks, Jack > > >> >> Thanks again for your review. Tag, you're it... >> >> Thanks, >> Jack >> >>
