Received: by 10.52.89.73 with SMTP id bm9mr216216vdb.3.1335302504013;
        Tue, 24 Apr 2012 14:21:44 -0700 (PDT)
X-BeenThere: chromium-os-revi...@chromium.org
Received: by 10.220.66.3 with SMTP id l3ls4672118vci.9.gmail; Tue, 24 Apr 2012
 14:21:43 -0700 (PDT)
Received: by 10.52.68.164 with SMTP id x4mr125703vdt.16.1335302503219;
        Tue, 24 Apr 2012 14:21:43 -0700 (PDT)
Received: by 10.52.68.164 with SMTP id x4mr125701vdt.16.1335302503206;
        Tue, 24 Apr 2012 14:21:43 -0700 (PDT)
Return-Path: <ger...@chromium.org>
Received: from ns1.golo.chromium.org ([74.125.248.94])
        by mx.google.com with ESMTP id hj4si13095629vdb.20.2012.04.24.14.21.42;
        Tue, 24 Apr 2012 14:21:43 -0700 (PDT)
Received-SPF: pass (google.com: domain of ger...@chromium.org designates 74.125.248.94 as permitted sender) client-ip=74.125.248.94;
Authentication-Results: mx.google.com; spf=pass (google.com: domain of ger...@chromium.org designates 74.125.248.94 as permitted sender) smtp.mail=ger...@chromium.org
Message-Id: <4f971967.646c340a.1cd4.76d9SMTPIN_ADDED@mx.google.com>
Received: from 192.168.20.14 (gerrit.golo.chromium.org [192.168.20.14])
	by ns1.golo.chromium.org (Postfix) with ESMTP id AAC02161565;
	Tue, 24 Apr 2012 14:21:42 -0700 (PDT)
Date: Tue, 24 Apr 2012 14:21:42 -0700
From: "Chris Sosa (Code Review)" <ger...@chromium.org>
To: Brian Harring <ferri...@chromium.org>
CC: David James <davidja...@chromium.org>
Reply-To: s...@chromium.org
X-Gerrit-MessageType: comment
Subject: =?UTF-8?Q?CQ:_Break_out_repo/path_series_application_into_a_standalone...__[chromiumos/chromite_:_master]=0A?=
X-Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Mailing-List: list gerrit-chromiumos-chrom...@gerrit.chromium.org
List-Id: <gerrit-chromiumos-chromite.gerrit.chromium.org>
List-Unsubscribe: <https://gerrit.chromium.org/gerrit/settings>
X-Gerrit-ChangeURL: <https://gerrit.chromium.org/gerrit/20504>
X-Gerrit-Commit: 8378b141dffeee26aad5f430b9e2baad734b94a7
In-Reply-To: <gerrit.1334785383609.I9b042c05f533eddef080949f1f1af30a77d4b4ff@gerrit.chromium.org>
References: <gerrit.1334785383609.I9b042c05f533eddef080949f1f1af30a77d4b...@gerrit.chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.1.8

Chris Sosa has posted comments on this change.

Change subject: CQ: Break out repo/path series application into a standalone object.
......................................................................


Patch Set 9: (6 inline comments)

....................................................
File buildbot/validation_pool.py
Line 69:     if external == True:
Ah I see the issue.

I don't like the idea of False, True, or Object.  It tends not to work well and be used wrong by others trying to maintain the code.

You are forgetting that unittests can do magical things to give you the functionality you want in Python.

mock_helper = self.mox.CreateMock(helper)
p = HelperPool()
p._external = mock_helper

Remember referencing and setting private variables in unittests is a-ok style-wise for Python if it makes your implementation more clear.

Line 88:     raise Exception("Internal bug detected: change %s (internal? %r) asked "
Assert it.

Line 94:       if helper is not None:
I'm not that strong about this one.  In general good python style dictates to keep things as short and clear as possible.  If you strongly feel this is correct / true in this case I won't object :)

Line 109:     self.GetHelper = helper_pool.ForChange
The key issue is you kill any help(validiation_pool) usage or any pydoc generating things as this method won't be documented/found.

I agree it's a nice short-hand but I prefer if we only leave those shorthands in unittests.

Line 265:   Usage:  Use ValidationPoo.AcquirePool -- a static
Hai.

Line 532:     applied, failed_tot, failed_inflight = \
Alas you misread my comment.  I explicitly meant avoid using the '\' character here :)

--
To view, visit https://gerrit.chromium.org/gerrit/20504
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b042c05f533eddef080949f1f1af30a77d4b4ff
Gerrit-PatchSet: 9
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferri...@chromium.org>
Gerrit-Reviewer: Brian Harring <ferri...@chromium.org>
Gerrit-Reviewer: Chris Sosa <s...@chromium.org>
Gerrit-Reviewer: David James <davidja...@chromium.org>