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
