Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
49653002:
Add push-to-trunk python port. (Closed)
|
Created:
7 years, 1 month ago by Michael Achenbach Modified:
7 years, 1 month ago Reviewers:
Jakob Kummerow CC:
v8-dev Visibility:
Public. |
DescriptionAdd push-to-trunk python port.
Preliminary version.
TODO: Add wrapper script in tools dir.
TODO: Some refactoring.
R=jkummerow@chromium.org
Committed: https://code.google.com/p/v8/source/detail?r=17600
Patch Set 1 #
Total comments: 76
Patch Set 2 : Addressed review comments #Patch Set 3 : Fixed options. #Patch Set 4 : Fixed read line. #
Total comments: 10
Patch Set 5 : Different fixes based on live test results. #Patch Set 6 : Addressed review comments. #Patch Set 7 : Addressed more review comments. #Patch Set 8 : Inlined command. #
Total comments: 4
Patch Set 9 : Review. #
Messages
Total messages: 9 (0 generated)
PTAL
Yo dawg, I herd you like comments. https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_in... File tools/push-to-trunk/common_includes.py (right): https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_t... File tools/push-to-trunk/push_to_trunk.py (right): https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_scri... File tools/push-to-trunk/test_scripts.py (right): https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/test_scri... 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_scri... 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_scri... 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_scri... 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_scri... 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_scri... 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_scri... 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.
PTAL https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_in... File tools/push-to-trunk/common_includes.py (right): https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_in... 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_t... File tools/push-to-trunk/push_to_trunk.py (right): https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_t... 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_scri... File tools/push-to-trunk/test_scripts.py (right): https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/test_scri... 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_scri... 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_scri... 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_scri... 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_scri... 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_scri... 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_scri... 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.
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_in... File tools/push-to-trunk/common_includes.py (right): https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_in... 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_t... File tools/push-to-trunk/push_to_trunk.py (right): https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_t... 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/comm... File tools/push-to-trunk/common_includes.py (right): https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/comm... 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... File tools/push-to-trunk/push_to_trunk.py (right): https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/push... 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... 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... 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... File tools/push-to-trunk/test_scripts.py (right): https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/test... 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)))
PTAL https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_in... File tools/push-to-trunk/common_includes.py (right): https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/common_in... 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_t... File tools/push-to-trunk/push_to_trunk.py (right): https://codereview.chromium.org/49653002/diff/1/tools/push-to-trunk/push_to_t... 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/comm... File tools/push-to-trunk/common_includes.py (right): https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/comm... 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... File tools/push-to-trunk/push_to_trunk.py (right): https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/push... 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... 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... 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... File tools/push-to-trunk/test_scripts.py (right): https://codereview.chromium.org/49653002/diff/150001/tools/push-to-trunk/test... 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.
PTAL at patch 8.
LGTM. https://codereview.chromium.org/49653002/diff/330001/tools/push-to-trunk/comm... File tools/push-to-trunk/common_includes.py (right): https://codereview.chromium.org/49653002/diff/330001/tools/push-to-trunk/comm... tools/push-to-trunk/common_includes.py:76: return subprocess.check_output("%s %s %s" % (prefix, cmd, args), shell=True) nit: 80col (I'd break after '%') https://codereview.chromium.org/49653002/diff/330001/tools/push-to-trunk/push... File tools/push-to-trunk/push_to_trunk.py (right): https://codereview.chromium.org/49653002/diff/330001/tools/push-to-trunk/push... tools/push-to-trunk/push_to_trunk.py:126: # TODO(machenbach): Handle multiple entries (e.g. BUG=123, 234). Agreed, this is one of the candidates for future improvement. The entire workflow from lines 121 to 134 should be a helper function that takes the output of "git log" and returns a beautifully formatted "(issue[s] A, B, C; Chromium issue[s] D, E, F)" string. With proper tests for pathological cases (including auto-detection of a missing "v8:" prefix!) and all other bells and whistles you can think of :-) Oh, and while we're at it, line breaks and trailing full stops should be fixed globally, i.e. instead of this: Fixed the foo when barring. (issue 123,chromium:456) I want this: Fixed the foo when barring (issue 123, Chromium issue 456).
https://codereview.chromium.org/49653002/diff/330001/tools/push-to-trunk/comm... File tools/push-to-trunk/common_includes.py (right): https://codereview.chromium.org/49653002/diff/330001/tools/push-to-trunk/comm... tools/push-to-trunk/common_includes.py:76: return subprocess.check_output("%s %s %s" % (prefix, cmd, args), shell=True) On 2013/11/08 14:01:18, Jakob wrote: > nit: 80col (I'd break after '%') Done. https://codereview.chromium.org/49653002/diff/330001/tools/push-to-trunk/push... File tools/push-to-trunk/push_to_trunk.py (right): https://codereview.chromium.org/49653002/diff/330001/tools/push-to-trunk/push... tools/push-to-trunk/push_to_trunk.py:126: # TODO(machenbach): Handle multiple entries (e.g. BUG=123, 234). On 2013/11/08 14:01:18, Jakob wrote: > Agreed, this is one of the candidates for future improvement. The entire > workflow from lines 121 to 134 should be a helper function that takes the output > of "git log" and returns a beautifully formatted "(issue[s] A, B, C; Chromium > issue[s] D, E, F)" string. With proper tests for pathological cases (including > auto-detection of a missing "v8:" prefix!) and all other bells and whistles you > can think of :-) > > Oh, and while we're at it, line breaks and trailing full stops should be fixed > globally, i.e. instead of this: > > Fixed the foo when barring. > (issue 123,chromium:456) > > I want this: > > Fixed the foo when barring (issue 123, Chromium issue 456). Postponed to future CL. The line break is taken care of when this goes into the commit msg. There is just no automatic (re)placement of the dot yet.
Message was sent while issue was closed.
Committed patchset #9 manually as r17600 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
