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#newcode74
tools/push-to-trunk/common_includes.py:74: p = subprocess.Popen("%s %s
%s" % (prefix, cmd, args),
On 2013/11/06 16:37:23, Jakob wrote:
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.
Same result. Try:
python -c "import subprocess; subprocess.check_output('vi')"
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
On 2013/11/06 16:37:23, Jakob wrote:
nit: typo
Done.
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):
On 2013/11/06 16:37:23, Jakob wrote:
nit: please use the same spelling: if the field is called
...effects...
(plural), then the setter should mirror that.
Done.
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):
On 2013/11/06 16:37:23, Jakob wrote:
maybe "raise NotImplementedError" here to clarify that subclasses must
override
this method.
Done.
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/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.
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)
On 2013/11/06 16:37:23, Jakob wrote:
nit: fits on last line
Done.
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
On 2013/11/06 16:37:23, Jakob wrote:
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.
Good to know. I'll also replace all the sys.stdout.write statements that
I used for that purpose.
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):
On 2013/11/06 16:37:23, Jakob wrote:
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.
Separate CL.
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__"
On 2013/11/06 16:37:23, Jakob wrote:
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.
Separate CL.
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)
On 2013/11/06 16:37:23, Jakob wrote:
nit: 80col. It's probably enough to delete the superfluous space after
"self.Die(".
Done.
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):
On 2013/11/06 16:37:23, Jakob wrote:
Let's match only once here.
match = re.match(r"^## (.+)", line)
if match:
current_branch = match.group(1)
Done.
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):
On 2013/11/06 16:37:23, Jakob wrote:
same "match only once" comment applies here.
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#newcode82
tools/push-to-trunk/push_to_trunk.py:82: loop = 1
On 2013/11/06 16:37:23, Jakob wrote:
Let's make this a bit more Pythonic.
s/loop = 1//
s/while loop == 1/while True/
s/loop = 0/break/
s/else://
Done. I tried to stay really close to the bash ;)
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)
On 2013/11/06 16:37:23, Jakob wrote:
Why do we need multiline substitution...
Well...
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):
On 2013/11/06 16:37:23, Jakob wrote:
...if we're mapping the function onto every line?
... we don't. That whole thing here could probably be shortened. It is
like this to be closer to the grep and pipe structure from bash.
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.
On 2013/11/06 16:37:23, Jakob wrote:
outdated
Done.
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 '{\
On 2013/11/06 16:37:23, Jakob wrote:
Don't you want to route this command through Step.CommandSE?
Since it has no side effects and since it is quite deterministic, not
necessarily. I didn't really intend to also mock out all the side effect
free stuff...
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' "
On 2013/11/06 16:37:23, Jakob wrote:
nit: I'd format as:
self.Die("Checking out a new branch '%s' failed." %
self.Config(TRUNKBRANCH))
Done.
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*$",
On 2013/11/06 16:37:23, Jakob wrote:
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)
Done.
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)
On 2013/11/06 16:37:23, Jakob wrote:
It's safe to assume that patch is always 0.
Done.
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))
On 2013/11/06 16:37:23, Jakob wrote:
another command that I think should go through the side effects
handler
The whole side effects with files, their creation, modification and
deletion is solved by mocking out all paths with tmp locations through
the Config. Like that the script can behave as in prod when tested.
Otherwise a virtual file system must be set up.
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)"
On 2013/11/06 16:37:23, Jakob wrote:
what's this?
The sys.stdout.write was a way to print without line ending (before I
got enlightened)
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 "
On 2013/11/06 16:37:23, Jakob wrote:
This is inconsistent with the print statements elsewhere. Please
decide for one
and use it consistently.
Done.
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.")
On 2013/11/06 16:37:23, Jakob wrote:
nit: we only checked for existence. s/ or not writable//.
Done.
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"]
On 2013/11/06 16:37:23, Jakob wrote:
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)
Done.
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):
On 2013/11/06 16:37:23, Jakob wrote:
s/hanlder/handler/
Done.
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/06 16:37:23, Jakob wrote:
Would it make sense to pass all these to the constructor?
But then I need some argw** passing from all subconstructors to the step
constructor, and there are quite many places.
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:]:
On 2013/11/06 16:37:23, Jakob wrote:
This is remarkably easy :-)
Done.
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",
On 2013/11/06 16:37:23, Jakob wrote:
feel free to add long names while you're here: --step, --lastpush,
--chromium.
Bash's option parser only supports one-character option names.
Done.
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 {}
On 2013/11/06 16:37:23, Jakob wrote:
shorter:
state = state or {}
It was like that because of a test that checked the side effects on the
initial state. And {} evaluates to false and gets overwritten with a
local {}. For a better design, I rewrote that test now.
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]
On 2013/11/06 16:37:23, Jakob wrote:
nit: git_invocation
Done.
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)))
On 2013/11/06 16:37:23, Jakob wrote:
nit: break after '%'
Done.
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,
On 2013/11/06 16:37:23, Jakob wrote:
nit: s/imput/input/, break after '%'
Done.
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"))
On 2013/11/06 16:37:23, Jakob wrote:
why not assertEquals(..., "git version 1.2.3")?
Done.
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):
On 2013/11/06 16:37:23, Jakob wrote:
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.
This is rather a playground, where you can go back to when the regexes
are not working as expected. I could keep this private to myself, but it
woun't hurt when it's checked in either.
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 "
On 2013/11/06 16:37:23, Jakob wrote:
I'd break the line after every "\n" to make it obvious that the
indentation is
consistently 8 spaces.
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.
