John Sonnenschein wrote:
> How's this look: http://cr.opensolaris.org/~error404/monaboo/
>
> a list given to __init__ that ranks the priority. default is to only
> check bugster.
>
> When it loops through all the bugs from whatever's in results[0] it
> removes them from crs and then tries the next bugdb down the list ( with
> the remainder ).
In the following:
76 if forceBoo:
77 self.__onSWAN = False
78 else:
79 self.__onSWAN = onSWAN()
80 if self.__onSWAN:
81 self.__m = self.__monaco
82 self.__priority = priority
- self.__m appears to be unused
- the conditional could reasonably be written as
self.__onSWAN = not forceBoo and onSWAN()
(or you could parenthesize ( not forceBoo ) if you don't like the
implicit precedence.)
The comment "do nothing. If bugzilla is set then next loop it'll be
caught" in lookup() seems a bit premature, in the absence of a
"bugzilla" lookup routine.
In line 184 (and 186), you reference (and return) results, but if the
priority setting is botched, it will be undefined.
The purpose of the loop on 182 appears to be avoiding lookup of bugs in
multiple systems. And that seems to jibe reasonably with the use of the
priority list. At a minimum, this should be commented, and bonus points
if you allow this to be defeated.
If you wrote __boobug() to iterate on a list and return a dict, then the
entire logic from 167-181 could be collapsed into calling a predefined
lookup function for each database. (And the onSWAN setting above would
set the appropriate lookup routine for bugster.) (In other words, it
seems like your various lookup routines should have identical function
prototypes, and currently they do not.)
Neither __monaco() nor __boobug() appears to handle an empty cr list
gracefully, but you have no logic to prevent that from happening, once
you add multiple lookup databases.
--Mark
> On 17-Sep-08, at 11:37 AM, Mark J. Nelson wrote:
>
>> John Sonnenschein wrote:
>>> Hey again,
>>> I need another code review ( on top of the one I need for
>>> http://cr.opensolaris.org/~error404/140/ )
>>
>> I replied to that one from the original request.
>>
>>> new code:
>>> http://cr.opensolaris.org/~error404/122/
>>
>> - Aren't you missing an "else" and some indentation before line 181?
>> It looks like you're always looking stuff up on BOO, even on SWAN,
>> even when Monaco succeeds.
>>
>> - The assignments on lines 102-111 could easily be replaced by mapping
>> desired field names to boo field names.
>>
>> - I'm not thrilled with the explicit fallback from Monaco and/or BOO
>> to DOO, especially not in the BugDB lookup method. I think the caller
>> should be able to specify where to look for bugs, and how to
>> prioritize those sources of info. See below.
>>
>> So the way I had envisioned something like this working would be to
>> have two classes, something like "BugInfoServer" and "BugDB."
>>
>> The BugDB initializer would take a list of BugInfoServer objects, in
>> priority order. For each lookup, test for validity of the bugid
>> against each BugInfoServer (presumably via a class method) in turn,
>> until you have exhausted the search space. Not sure whether a valid
>> return should mean "stop searching," that probably depends on whether
>> a bugid may be valid in multiple info servers.
>>
>> The BugInfoServer class would have (at least) two subclasses: Bugster
>> (which would encompass both Monaco and BOO, depending on SWAN access)
>> and Bugzilla (which would take a server argument, probably defaulting
>> to DOO).
>>
>> Am I making this too complex and general?
>>
>>> and the tests fail, but I'm blaming the tests, they try to touch
>>> boobug and monaco directly, which have ceased to exist. New tests:
>>> http://cr.opensolaris.org/~error404/122-scmtest/
>>> The tests fail when I'm on SWAN, but they fail on RTI lookups, which
>>> I didn't touch and they fail on a completely clean tree so I'm going
>>> to ignore them now ( and fix them later ).
>>
>> We (not sure of the exact antecedent there) need, I suspect, to file
>> another test bug for our own use only (and make it very obviously ON
>> tools test specific), then file an RTI for it.
>>
>> We were previously using bug 6227559, covered by RTI 300560. But on
>> September 3, somebody else filed two more RTIs referencing this bug,
>> resulting in test failures for us, because now we're seeing RTIs in
>> multiple consolidations.
>>
>> Or we go do something to those other RTIs, but the folks that filed
>> 'em didn't do so maliciously.
>>
>>> I'm not sure where I got the bug # 122 from... ignore it I guess
>>> Thanks people
>>> -JohnS
>>> _______________________________________________
>>> scm-migration-dev mailing list
>>> scm-migration-dev at opensolaris.org
>>> http://mail.opensolaris.org/mailman/listinfo/scm-migration-dev
>>
>
