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). 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)
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.
This probably deserves a separate bug.
245-246: long() won't accept 'decimal' values, only integers, so this
should state that it's not a valid 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)
119: findAIfromCriteria() doesn't ever return anything, meaning it
implicitly returns None. This "if" block will never get executed.
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.
236,239: For clarity, can we wrap ('MIN' + crit) and ('MAX' + crit) in
parentheses?
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]
306: Should be "is None" (unless you use my suggestion above, which
eliminates the need for this check)
329: Wrap this in parentheses and remove the \
446: Should be "is not None"
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?
482: Trailing \ no longer needed
542: if source == "AI":
545: Should probably be "if self._criteriaRoot is None:"
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")
549: This should be "if root:" (actually, this 'if' statement seems
entirely unnecessary)
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?
572: Should probably be: "if self._criteriaRoot is None:"
615: "is not None"
624: "is None"
633: "is None"
642: "if SCman.attrib['name'] in self._smfDict:" (call to keys() is
unneeded)
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)
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.
707: "is None"
732: "is not None"
747: "is not None"
756: "is None"
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
