Yup, better. I'll wait for your other upcoming fixes before officially
approving.
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/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?
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/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 :-)
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)
nit: "major" is a misleading name. "value" would be better.
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*$",
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.
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"])
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.)
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)"
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)"
;-)
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)))
nit: for style bonus points, align as:
raise Exception("Called git too seldom: %d vs. %d" %
(self._git_index, len(self._git_recipe)))
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.
