Hi Clay, Comments in-line.
Clay Baenziger wrote: > Hi Keith, > Thank you for the in-depth code review. My comments in-line. I've > marked what is traditionally in-scope and what's out of scope too just > as an FYI -- regardless it's very good if you have the time for all of > this feedback. > > New differential webrev at: > http://cr.opensolaris.org/~clayb/publish_manifest/webrev2 > > Thank you, > Clay > > On Fri, 23 Oct 2009, Keith Mitchell wrote: > >> Code change comments: >> >> General: The usage of SystemExit throughout seems inappropriate. >> Functions which have no context of program execution shouldn't be >> raising these - they should be raising appropriate ValueErrors, >> TypeErrors, KeyErrors, etc. Callers into those functions (or callers >> of those callers) should be wrapping appropriate try/except blocks >> (or not wrapping, if they're not equipped to handle it, in which case >> the caller of the caller should be prepared to handle it, and so on). > > Dave had similar concerns when this code was written, see: > Bug 4016 - A/I Python code error architecture should be looked at for > better reuse (i.e. in a different U/I) Thanks for the pointer to this bug. I tried to find something along those lines, but didn't see it. > >> As a general rule, we should be raising and catching sub-classes of >> Exception or StandardError, not BaseException (see >> http://docs.python.org/library/exceptions.html#exception-hierarchy) > > My understanding is that we should sub-class from Exception or > StandardError, but there's nothing wrong with the built-in exceptions > based off BaseException. Am I missing a section in this document? Correct. I linked to that diagram because the terminology of "Exception" and "BaseException" is easily confusing (I truly wish that "Exception" were named differently, as it makes it difficult to discuss errors and exceptions clearly) > >> It seems the use of SystemExit throughout was intended to avoid >> printing the stacktrace. If that's true, then a better behavior would >> be to perhaps have something in the main function that catches all >> Exceptions and prints an appropriate one-liner - especially given >> that we're doing this throughout the code anyway, frequently catching >> raised exceptions just to raise a SystemExit (which hides some data >> that could be useful for debugging). We can then print traceback data >> directly to the log if desired. > > Yes, though delete_service.py in the gate still heavily relies on > SystemExit I think a wrapper around for execution would be a better > approach. > >> This probably deserves a separate bug. > > Indeed, as I'm not touching error handling here I don't want to enter > the ball of wax of re-engineering the error handling for this wad o' > fixes. > > ----- in scope below, thank you ---- > >> 245-246: long() won't accept 'decimal' values, only integers, so this >> should state that it's not a valid integer. > > Thank you, I was thinking decimal as in the base but have changed to > integer. > >> PEP-8 comments (publish-manifest) >> >> General: Is it feasible to rename these functions to camel case, or >> would that be too far reaching / out of scope? Same question for >> variable names within functions (local variables, at least, should be >> less disruptive) > > I am planning to finish the PEP8-ification of these files with the > Python versioning work being done in the next month (I'm the > responsible engineer for these components) but just wanted to get > these fixes pushed for now. That works. Renaming functions has larger implications than simple whitespace changes, etc, so I've no problem with delaying it. > > ---- out of scope below, but thank you still --- > >> 119: findAIfromCriteria() doesn't ever return anything, meaning it >> implicitly returns None. This "if" block will never get executed. > > Yes, this is vestigial from when one could pass a different AI and SC > manifest into publish-manifest. I've removed this code block. > >> 200-210: This block is pretty difficult to read, and seems to cover 2 >> different error conditions - splitting it up would make it more >> legible and provide more concise indications of what caused the error. > > I did add some comments to make the if statements clearer That helps, thanks > >> 236,239: For clarity, can we wrap ('MIN' + crit) and ('MAX' + crit) >> in parentheses? > > That makes it confusing to me, as there's no (functional) reason to > have the parens when I read that. Fair enough. I only brought it up because I originally read it as "MIN" + (crit not in ...), and it seemed like adding 'non-functional' parentheses would make it overly explicit. > >> 305: Splitting on the '.' is ugly and hard to read. This block of >> code also seems to repeatedly use crit.replace() - can we streamline >> this a bit? This would be identical and much cleaner: >> >> crit_key = crit.replace('MIN', '', 1).replace('MAX', '', 1) >> manCriteria = files.getCriteria(crit_key) >> if crit.startswith('MIN'): >> manCriteria = manCriteria[0] >> elif crit.startswith('MAX'): >> manCriteria = manCriteria[1] > > I agree that's nearly functionally the same and much cleaner! I've put > it in place. (But remember if manCriteria is set to None in the > initial call to getCriteria that taking a slice of None later in the > if/elif would be bad. Good point. Followup: You've got a typo now on lines 307 and 309 - you put ma"c"Criteria instead of ma"n"Criteria > >> 306: Should be "is None" (unless you use my suggestion above, which >> eliminates the need for this check) > > I grabbed your code from above. > >> 329: Wrap this in parentheses and remove the \ > > I prefer the \ here (otherwise the line continue in line with the > first statement after the if which isn't so readable). Ok. > >> 446: Should be "is not None" > > Indeed > >> 468: As long as we're changing this, can't we just use >> oldManifest.read(), which returns the full text, instead of using >> readlines and then joining them? > > I'd prefer not to change this, however, bug 4192 (publish-manifest > should use LXML doctest instead of md5 for XML comparisons) will. Ok. > >> 482: Trailing \ no longer needed > > Thank you > >> 542: if source == "AI": > > Hrm, good thing that is vestigial (I think). Fixed. > >> 545: Should probably be "if self._criteriaRoot is None:" > > Ah so on line 545 of the post-PEP8 changes, pre-bugfix but this is the > opposite logic. Though this change seems un-necessary even if changed > to is not None as I want this to only be true if this has a value, of > not None, not "", not some other semi-exists but no data here type. Sorry, I meant to type that as "is not None", but you're correct (that's why I hedged my bets with 'probably' as I wasn't 100% sure of the intent of this line). > >> 542-548 could be converged to: >> if self._criteriaRoot is None or source == "AI": >> root = self._AIRoot.findall(".//ai_criteria") >> else: >> root = self._criteriaRoot.findall(".//ai_criteria") > > Good collapse. Given your comments above about _criteriaRoot, this should actually be 'if not self._criteriaRoot or source == "AI":' instead of comparing to None. > >> 549: This should be "if root:" (actually, this 'if' statement seems >> entirely unnecessary) > > True > >> 537-565: Sidenote - this is really strange syntax. If this is a >> generator, it should raise GeneratorExit, and not have a return. If >> it's not a generator, is there a reason it's yielding? > > I think this is standard PEP255 syntax. It would be odd syntax were it > following the co-routine work in PEP342 but it doesn't need any of > that fanciness. However, as with your last comment and this, I've > removed the if/else and last return, as per PEP255, it wasn't necessary. Thanks for the pointer about PEP255. I'd only seen syntaxes that made use of StopIteration (not GeneratorExit, which was incorrect in my initial comment). > >> 572: Should probably be: "if self._criteriaRoot is None:" > > Same as the suggestion for 545. > >> 615: "is not None" >> 624: "is None" >> 633: "is None" > > And I think a few more even! But I've converted all ==/!= None's to > is/is not None. Excellent. > >> 642: "if SCman.attrib['name'] in self._smfDict:" (call to keys() is >> unneeded) > > Yes, thank you -- my first attempt at Python last year :) > Two other occurrences in the file too. Perfect. > >> 662: Splitting this up into multiple lines would improve readability: >> xmlData = lxml.etree.tostring(SCman.getchildren()[0]) >> xmlData = xmlData.replace("<!-- ", "").replace(" -->", "") >> xmlData = StringIO.StringIO(xmlData) > > But now it's not a three line intractable ball of wax! Yes, fixed -- > thank you. > >> 672: Should probably be: "if self._criteriaRoot is None:" >> * We use this check often enough that it seems we should have a >> property called 'source' or 'root' or something that returns either >> _criteriaRoot or _AIRoot dynamically. > > Yes, though rather a trivial test too... Yup. My mind tends to overanalyze some of this things... > >> 707: "is None" >> 732: "is not None" >> 747: "is not None" >> 756: "is None" > > Haha, yes I think I've gotten them all replaced. Great! Thanks again for being patient with me while I go on crusades in our Python code... - Keith > >> Clay Baenziger wrote: >>> Hi all, >>> If anyone has a few moments to review some PEP8 changes and a 20 >>> line bug fix, I'd love to push some code before folks start pushing >>> for our Python 2.6 blitz. I've fixed the following UI bugs in >>> publish-manifest and a typo in the default A/I manifest: >>> >>> 4318 - publish-manifest needs to error about no criteria in >>> non-default >>> manifests >>> 4326 - Ugly error if wrong data provided in criteria manifest >>> 11984 - Typo in suggestion in AI's default.xml manifest >>> >>> To show how differential webrevs can work for PEP8 changes see the >>> break out of webrevs below: >>> >>> Full Webrev (big): >>> http://cr.opensolaris.org/~clayb/publish_manifest >>> >>> PEP8 Changes: >>> http://cr.opensolaris.org/~clayb/publish_manifest/pep8 >>> >>> Bug Fixes (only 21 lines!): >>> http://cr.opensolaris.org/~clayb/publish_manifest/diff >>> >>> Bugs: >>> publish-manifest needs to error about no criteria in non-default >>> manifests: >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4318 >>> >>> Ugly error if wrong data provided in criteria manifest >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4326 >>> >>> Typo in suggestion in AI's default.xml manifest >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=11984 >>> >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>
