Yo dawg, I herd you like comments.
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#newcode74
tools/push-to-trunk/common_includes.py:74: p = subprocess.Popen("%s %s
%s" % (prefix, cmd, args),
Have you tried using subprocess.check_output()? The test runner uses
that for various git/shell calls, and it seems to be a bit easier to use
than wrapping Popen() by hand.
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py#newcode99
tools/push-to-trunk/common_includes.py:99: self._side_effects_hanlder =
DEFAULT_SIDE_EFFECTS_HANDLER
nit: typo
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py#newcode113
tools/push-to-trunk/common_includes.py:113: def
SetSideEffectHandler(self, handler):
nit: please use the same spelling: if the field is called ...effects...
(plural), then the setter should mirror that.
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py#newcode131
tools/push-to-trunk/common_includes.py:131: def RunStep(self):
maybe "raise NotImplementedError" here to clarify that subclasses must
override this method.
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):
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?
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py#newcode145
tools/push-to-trunk/common_includes.py:145: args, pipe=False)
nit: fits on last line
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py#newcode154
tools/push-to-trunk/common_includes.py:154: print "%s [Y/n] " % msg
pro tip: if you add a comma:
print "%s [Y/n] " % msg,
that'll suppress the line break that Python automatically inserts at the
end of a print statement's output.
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py#newcode171
tools/push-to-trunk/common_includes.py:171: def Persist(self, var_name,
value):
Per-variable persist/restore is fine for now if you want to stay close
to the bash version. Medium-term, we should just write/read all of
self._state, encoded as JSON or whatever.
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py#newcode172
tools/push-to-trunk/common_includes.py:172: value = value or "__EMPTY__"
IIRC __EMPTY__ was a hack around the fact that Bash can't properly deal
with empty strings. Python can. So in the bright future, we can remove
this hack.
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py#newcode177
tools/push-to-trunk/common_includes.py:177: value = value or self.Die(
"Variable '%s' could not be restored." % var_name)
nit: 80col. It's probably enough to delete the superfluous space after
"self.Die(".
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py#newcode203
tools/push-to-trunk/common_includes.py:203: if re.match(r"^##.*", line):
Let's match only once here.
match = re.match(r"^## (.+)", line)
if match:
current_branch = match.group(1)
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_includes.py#newcode233
tools/push-to-trunk/common_includes.py:233: if re.match(r"^#define
%s\s+\d*" % def_name, line):
same "match only once" comment applies here.
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#newcode82
tools/push-to-trunk/push_to_trunk.py:82: loop = 1
Let's make this a bit more Pythonic.
s/loop = 1//
s/while loop == 1/while True/
s/loop = 0/break/
s/else://
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode129
tools/push-to-trunk/push_to_trunk.py:129: text = MSub(r"BUG=v8:(.*)$",
r"issue \1", text)
Why do we need multiline substitution...
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode134
tools/push-to-trunk/push_to_trunk.py:134: for line in map(FormatIssue,
out):
...if we're mapping the function onto every line?
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode159
tools/push-to-trunk/push_to_trunk.py:159: # Eliminate any trailing
newlines by going through a shell variable.
outdated
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode225
tools/push-to-trunk/push_to_trunk.py:225: TextToFile(Command("cat %s |
awk --posix '{\
Don't you want to route this command through Step.CommandSE?
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode304
tools/push-to-trunk/push_to_trunk.py:304: self.Die("Checking out a new
branch '%s' "
nit: I'd format as:
self.Die("Checking out a new branch '%s' failed." %
self.Config(TRUNKBRANCH))
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode324
tools/push-to-trunk/push_to_trunk.py:324: text = MSub(r"(?<=#define
MAJOR_VERSION)(?P<space>\s+)\d*$",
I hate this syntax. It's OK to land this as is for now to get
testability, but I really want to see this rewritten soon to something
that's more readable. Roughly:
for line in VERSION_FILE:
if line.startswith("#define MAJOR_VERSION"):
line = line.replace("\d*$", new_major)
elif line.startswith("#define MINOR_VERSION"):
...
output += line
WriteToFile(output, VERSION_FILE)
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode331
tools/push-to-trunk/push_to_trunk.py:331: r"\g<space>%s" %
self._state["patch"], text)
It's safe to assume that patch is always 0.
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode345
tools/push-to-trunk/push_to_trunk.py:345: Command("rm", "-f %s*" %
self.Config(COMMITMSG_FILE))
another command that I think should go through the side effects handler
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode404
tools/push-to-trunk/push_to_trunk.py:404: print ">>> (asking for
Chromium checkout)"
what's this?
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode405
tools/push-to-trunk/push_to_trunk.py:405: sys.stdout.write("Do you have
a \"NewGit\" Chromium checkout and want "
This is inconsistent with the print statements elsewhere. Please decide
for one and use it consistently.
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode427
tools/push-to-trunk/push_to_trunk.py:427: self.Die("DEPS file not
present or not writable.")
nit: we only checked for existence. s/ or not writable//.
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode442
tools/push-to-trunk/push_to_trunk.py:442: args = "checkout -b
\"v8-roll-%s\"" % self._state["trunk_revision"]
I don't think you need the nested quotes here. The revision number will
never contain a space (or we'll have bigger problems anyway)
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode513
tools/push-to-trunk/push_to_trunk.py:513:
side_effects_hanlder=DEFAULT_SIDE_EFFECTS_HANDLER):
s/hanlder/handler/
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)
Would it make sense to pass all these to the constructor?
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode555
tools/push-to-trunk/push_to_trunk.py:555: for step in steps[options.s:]:
This is remarkably easy :-)
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_trunk.py#newcode561
tools/push-to-trunk/push_to_trunk.py:561: result.add_option("-s",
feel free to add long names while you're here: --step, --lastpush,
--chromium. Bash's option parser only supports one-character option
names.
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/test_scripts.py
File tools/push-to-trunk/test_scripts.py (right):
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/test_scripts.py#newcode74
tools/push-to-trunk/test_scripts.py:74: state = state if state is not
None else {}
shorter:
state = state or {}
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/test_scripts.py#newcode85
tools/push-to-trunk/test_scripts.py:85: git_invokation =
self._git_recipe[self._git_index]
nit: git_invocation
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/test_scripts.py#newcode130
tools/push-to-trunk/test_scripts.py:130: len(self._git_recipe)))
nit: break after '%'
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/test_scripts.py#newcode132
tools/push-to-trunk/test_scripts.py:132: raise Exception("Too little
imput: %d vs. %d" % (self._rl_index,
nit: s/imput/input/, break after '%'
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/test_scripts.py#newcode146
tools/push-to-trunk/test_scripts.py:146:
self.assertTrue(self.MakeStep().Git("--version").startswith("git
version"))
why not assertEquals(..., "git version 1.2.3")?
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/test_scripts.py#newcode204
tools/push-to-trunk/test_scripts.py:204: def testRegex(self):
This is better than having no test for the regexes at all, but in the
future I'd like to see tests that actually test the production code of
the EditChangeLog and SetVersion steps.
https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/test_scripts.py#newcode215
tools/push-to-trunk/test_scripts.py:215: self.assertEqual(" too
little\n tab tab\n too "
I'd break the line after every "\n" to make it obvious that the
indentation is consistently 8 spaces.
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.
