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)
>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?
> 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.
---- 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
> 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.
> 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.
> 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).
> 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.
> 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.
> 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.
> 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.
> 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.
> 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.
> 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...
> 707: "is None"
> 732: "is not None"
> 747: "is not None"
> 756: "is None"
Haha, yes I think I've gotten them all replaced.
> 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
>
