PTAL
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py
File tools/push-to-trunk/common_includes.py (right):
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py#newcode137
tools/push-to-trunk/common_includes.py:137: def CommandSE(self, cmd,
args="", prefix="", pipe=True):
On 2013/11/08 10:43:19, Jakob wrote:
On 2013/11/07 15:56:46, machenbach wrote:
> On 2013/11/06 16:37:23, Jakob wrote:
> > Hm... I'm not too happy with the -SE postfix. How do you feel
about calling
> this
> > just "Command" and routing all external commands through here?
>
> Done.
Sorry if I didn't express that clearly enough; my point was that
having to
distinguish at every call site whether a call can be mocked out or not
(and
calling either the global Command() or self.Command() accordingly) is
brittle
and unintuitive. It would be nice to have a single bottleneck that
then decides
whether a command should be passed through the side effect handler or
not. As
long as we do have two separate functions, giving them visibly
different names
is actually better. AFAICS the only call sites of this method are
Git() and
Editor() below, so you should mark it private by prepending an
underscore. In
that case I don't care much whether it has the -SE postfix or not. How
about
simply inlining it?
Done.
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py
File tools/push-to-trunk/push_to_trunk.py (right):
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode547
tools/push-to-trunk/push_to_trunk.py:547: step.SetNumber(number)
On 2013/11/08 10:43:19, Jakob wrote:
On 2013/11/07 15:56:46, machenbach wrote:
> But then I need some argw** passing from all subconstructors to the
step
> constructor, and there are quite many places.
I see your point. How about a factory method then? Feel free to punt
for now :-)
Punted.
https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/common_includes.py
File tools/push-to-trunk/common_includes.py (right):
https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/common_includes.py#newcode235
tools/push-to-trunk/common_includes.py:235: major = match.group(1)
On 2013/11/08 10:43:19, Jakob wrote:
nit: "major" is a misleading name. "value" would be better.
Done.
https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/push_to_trunk.py
File tools/push-to-trunk/push_to_trunk.py (right):
https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/push_to_trunk.py#newcode190
tools/push-to-trunk/push_to_trunk.py:190: text = MSub(r"(?<=#define
BUILD_NUMBER)(?P<space>\s+)\d*$",
On 2013/11/08 10:43:19, Jakob wrote:
This is another regexp construct I'd like to replace with something
more
readable, but it's fine to do that in a future CL.
Let's do that later, especially because it is a one-liner.
https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/push_to_trunk.py#newcode323
tools/push-to-trunk/push_to_trunk.py:323: line = line.replace("\d*$",
self._state["major"])
On 2013/11/08 10:43:19, Jakob wrote:
string.replace doesn't handle regexp patterns. You want:
line = re.sub("\d+$", self._state["major"], line)
Same below. (Note the "+", we are sure that there will be a non-zero
number of
digits.)
Done.
https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/push_to_trunk.py#newcode403
tools/push-to-trunk/push_to_trunk.py:403: print ">>> (asking for
Chromium checkout)"
On 2013/11/08 10:43:19, Jakob wrote:
My point was that this line is pointless. To be fully consistent,
you'd have to
insert at least one more line before it:
print ">>> (warning the user about impending Chromium checkout
question)"
;-)
Done.
https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/test_scripts.py
File tools/push-to-trunk/test_scripts.py (right):
https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/test_scripts.py#newcode130
tools/push-to-trunk/test_scripts.py:130: (self._git_index,
len(self._git_recipe)))
On 2013/11/08 10:43:19, Jakob wrote:
nit: for style bonus points, align as:
raise Exception("Called git too seldom: %d vs. %d" %
(self._git_index, len(self._git_recipe)))
Done.
https://codereview.chromium.org/49653002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
