Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23072039:
Fix R= line rewriter to handle TBRs well. (Closed)
|
Created:
7 years, 4 months ago by agable Modified:
7 years, 3 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Visibility:
Public. |
DescriptionFix R= line rewriter to handle TBRs well.
R=maruel@chromium.org, thakis@chromium.org
BUG=253589
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=222801
Patch Set 1 #
Total comments: 4
Patch Set 2 : Refactoring + tests #
Total comments: 18
Patch Set 3 : More comments. #
Total comments: 11
Patch Set 4 : Comments. #
Total comments: 18
Patch Set 5 : Nits #Messages
Total messages: 13 (0 generated)
PTAL. I think this solution is reasonable. There are three sources of input: the reviewers passed to the function (which are always either the people passed on the command line as "-r foo" or the people grabbed by the script as having sent "lgtm" in a rietveld message), the people on R= lines in the description, and the people on TBR= lines in the description. This takes the first two sources of input and combines them into one R= line, prioritizing the names passed on the command line or that have LGTM'd the change. It then takes the TBR people (if any) and puts them on a single line right after that R= line.
https://codereview.chromium.org/23072039/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/23072039/diff/1/git_cl.py#newcode887 git_cl.py:887: new_r_line = 'R=' + ', '.join(reviewers) I'd also skip it unless "if reviewers:" https://codereview.chromium.org/23072039/diff/1/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/23072039/diff/1/tests/git_cl_test.py#newcode697 tests/git_cl_test.py:697: ('foo\nR=xx\nTBR=yy\nR=bar', ['a@c'], 'foo\nR=a@c, xx, bar\nTBR=yy'), Add a test with no R= line, only a TBR= line and a random person lgtm'ing it.
PTAL. I actually ended up making some slightly more significant changes (revolving around storing _description as a list of lines rather than a big string), but I think it's all for the better. https://codereview.chromium.org/23072039/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/23072039/diff/1/git_cl.py#newcode887 git_cl.py:887: new_r_line = 'R=' + ', '.join(reviewers) On 2013/08/23 15:22:53, M-A Ruel wrote: > I'd also skip it unless "if reviewers:" I ended up refactoring most of the ChangeDescription class to make more sense. This change is included in that. https://codereview.chromium.org/23072039/diff/1/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/23072039/diff/1/tests/git_cl_test.py#newcode697 tests/git_cl_test.py:697: ('foo\nR=xx\nTBR=yy\nR=bar', ['a@c'], 'foo\nR=a@c, xx, bar\nTBR=yy'), On 2013/08/23 15:22:53, M-A Ruel wrote: > Add a test with no R= line, only a TBR= line and a random person lgtm'ing it. To go along with the refactoring, I added a bunch of logically progressing tests, this one included.
https://codereview.chromium.org/23072039/diff/7001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode855 git_cl.py:855: self._description = (description or '').strip().splitlines() Rename the variable to be sure. self._description_lines https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode859 git_cl.py:859: return '\n'.join(self._description).strip() I'd prefer to not strip here and store it stripped for consistency. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode872 git_cl.py:872: self._description = new_desc You also want to strip off ending lines, e.g. while self._description and not self._description[-1]: self._description.pop() https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode883 git_cl.py:883: tbr_names += people I prefer using explicitly .append() or .extend(). Otherwise it occasionally create hard-to-figure out bugs. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode886 git_cl.py:886: for name in r_names: Using a set() for reviewers would make this trivial. Also, I'd prefer for this code to not modify its input. It can create hard-to-track bugs. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode890 git_cl.py:890: new_tbr_line = 'TBR=' + ', '.join(tbr_names) if tbr_names else None This one is a problem, so people put "TBR=" on their CL description, it should not be removed on commit. This should be unit tested. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode922 git_cl.py:922: content = content.strip().splitlines() lines = https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode931 git_cl.py:931: if not self._description: if self._description: <lines 933-937> https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode943 git_cl.py:943: matches = [regexp.match(line) for line in self._description] it's fine to use re.match(self.R_LINE, line). The re module keeps a cache, so when you iterate over the same re, it's fast well enough for this use case.
https://codereview.chromium.org/23072039/diff/7001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode855 git_cl.py:855: self._description = (description or '').strip().splitlines() On 2013/08/23 18:53:39, M-A Ruel wrote: > Rename the variable to be sure. > self._description_lines Done. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode859 git_cl.py:859: return '\n'.join(self._description).strip() On 2013/08/23 18:53:39, M-A Ruel wrote: > I'd prefer to not strip here and store it stripped for consistency. Done. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode872 git_cl.py:872: self._description = new_desc On 2013/08/23 18:53:39, M-A Ruel wrote: > You also want to strip off ending lines, e.g. > while self._description and not self._description[-1]: > self._description.pop() Done, via a properties.setter that handles all stripping every time we write directly to self.description. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode883 git_cl.py:883: tbr_names += people On 2013/08/23 18:53:39, M-A Ruel wrote: > I prefer using explicitly .append() or .extend(). Otherwise it occasionally > create hard-to-figure out bugs. Done. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode886 git_cl.py:886: for name in r_names: On 2013/08/23 18:53:39, M-A Ruel wrote: > Using a set() for reviewers would make this trivial. Yes, but I'd like to keep the reviewers ordered (first the cmd-line reviewers, in order, then everyone else from the description, in order) because people often encode priority in reviewer orders. > Also, I'd prefer for this code to not modify its input. It can create > hard-to-track bugs. Done. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode890 git_cl.py:890: new_tbr_line = 'TBR=' + ', '.join(tbr_names) if tbr_names else None On 2013/08/23 18:53:39, M-A Ruel wrote: > This one is a problem, so people put "TBR=" on their CL description, it should > not be removed on commit. This should be unit tested. Not sure what you mean. If someone puts "TBR=" in their CL description (with no one listed after the =), what are we supposed to do with that? It certainly shouldn't count as valid review. I think that TAG=VALUE lines with no VALUE should be removed. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode922 git_cl.py:922: content = content.strip().splitlines() On 2013/08/23 18:53:39, M-A Ruel wrote: > lines = Done. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode931 git_cl.py:931: if not self._description: On 2013/08/23 18:53:39, M-A Ruel wrote: > if self._description: > <lines 933-937> Done. https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode943 git_cl.py:943: matches = [regexp.match(line) for line in self._description] On 2013/08/23 18:53:39, M-A Ruel wrote: > it's fine to use re.match(self.R_LINE, line). The re module keeps a cache, so > when you iterate over the same re, it's fast well enough for this use case. Done. Was just following the same pattern as REs elsewhere in this class. https://codereview.chromium.org/23072039/diff/12001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/23072039/diff/12001/tests/git_cl_test.py#newc... tests/git_cl_test.py:698: ('foo\nR=xx', ['a@c'], 'foo\n\nR=a@c, xx'), These test expectations changed because I realized that, when the R= line is the last line of the description and then we trim all trailing empty lines, there's no way to know if the original description had "foo\nR=..." or "foo\n\n\n\nR=...", so I went with the pretty thing and put a single empty line above the tag (see line 907 of git_cl).
On 2013/08/26 17:16:45, Aaron Gable wrote: > https://codereview.chromium.org/23072039/diff/7001/git_cl.py > File git_cl.py (right): > > https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode855 > git_cl.py:855: self._description = (description or '').strip().splitlines() > On 2013/08/23 18:53:39, M-A Ruel wrote: > > Rename the variable to be sure. > > self._description_lines > > Done. > > https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode859 > git_cl.py:859: return '\n'.join(self._description).strip() > On 2013/08/23 18:53:39, M-A Ruel wrote: > > I'd prefer to not strip here and store it stripped for consistency. > > Done. > > https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode872 > git_cl.py:872: self._description = new_desc > On 2013/08/23 18:53:39, M-A Ruel wrote: > > You also want to strip off ending lines, e.g. > > while self._description and not self._description[-1]: > > self._description.pop() > > Done, via a properties.setter that handles all stripping every time we write > directly to self.description. > > https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode883 > git_cl.py:883: tbr_names += people > On 2013/08/23 18:53:39, M-A Ruel wrote: > > I prefer using explicitly .append() or .extend(). Otherwise it occasionally > > create hard-to-figure out bugs. > > Done. > > https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode886 > git_cl.py:886: for name in r_names: > On 2013/08/23 18:53:39, M-A Ruel wrote: > > Using a set() for reviewers would make this trivial. > > Yes, but I'd like to keep the reviewers ordered (first the cmd-line reviewers, > in order, then everyone else from the description, in order) because people > often encode priority in reviewer orders. > > > Also, I'd prefer for this code to not modify its input. It can create > > hard-to-track bugs. > > Done. > > https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode890 > git_cl.py:890: new_tbr_line = 'TBR=' + ', '.join(tbr_names) if tbr_names else > None > On 2013/08/23 18:53:39, M-A Ruel wrote: > > This one is a problem, so people put "TBR=" on their CL description, it should > > not be removed on commit. This should be unit tested. > > Not sure what you mean. If someone puts "TBR=" in their CL description (with no > one listed after the =), what are we supposed to do with that? It certainly > shouldn't count as valid review. I think that TAG=VALUE lines with no VALUE > should be removed. > > https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode922 > git_cl.py:922: content = content.strip().splitlines() > On 2013/08/23 18:53:39, M-A Ruel wrote: > > lines = > > Done. > > https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode931 > git_cl.py:931: if not self._description: > On 2013/08/23 18:53:39, M-A Ruel wrote: > > if self._description: > > <lines 933-937> > > Done. > > https://codereview.chromium.org/23072039/diff/7001/git_cl.py#newcode943 > git_cl.py:943: matches = [regexp.match(line) for line in self._description] > On 2013/08/23 18:53:39, M-A Ruel wrote: > > it's fine to use re.match(self.R_LINE, line). The re module keeps a cache, so > > when you iterate over the same re, it's fast well enough for this use case. > > Done. Was just following the same pattern as REs elsewhere in this class. > > https://codereview.chromium.org/23072039/diff/12001/tests/git_cl_test.py > File tests/git_cl_test.py (right): > > https://codereview.chromium.org/23072039/diff/12001/tests/git_cl_test.py#newc... > tests/git_cl_test.py:698: ('foo\nR=xx', ['a@c'], 'foo\n\nR=a@c, xx'), > These test expectations changed because I realized that, when the R= line is the > last line of the description and then we trim all trailing empty lines, there's > no way to know if the original description had "foo\nR=..." or > "foo\n\n\n\nR=...", so I went with the pretty thing and put a single empty line > above the tag (see line 907 of git_cl). Ping. Though I also take off for a week of vacation soon, so I may not be able to respond for a week.
https://codereview.chromium.org/23072039/diff/12001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/23072039/diff/12001/git_cl.py#newcode862 git_cl.py:862: def description(self, desc): # pylint: disable=E0202 I must say I'm not a fan of keeping a setter here, it makes understand foo.description = <something> much harder to understand and reduce the general maintainability of this file. If someone wants to update the lines, ._description_lines is there. Another option is an explicit set_description() member method. https://codereview.chromium.org/23072039/diff/12001/git_cl.py#newcode864 git_cl.py:864: lines = desc.strip().splitlines() .strip() is not necessary because of lines 867-870. https://codereview.chromium.org/23072039/diff/12001/git_cl.py#newcode866 git_cl.py:866: lines = [line.strip() for line in desc] That's wrong, you are left-aligning everything. https://codereview.chromium.org/23072039/diff/12001/git_cl.py#newcode868 git_cl.py:868: lines = lines[1:] lines.pop(0) will just the same faster. https://codereview.chromium.org/23072039/diff/12001/git_cl.py#newcode870 git_cl.py:870: lines = lines.pop() That's wrong since you are keeping the last line; you want: lines.pop()
PTAL https://codereview.chromium.org/23072039/diff/12001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/23072039/diff/12001/git_cl.py#newcode862 git_cl.py:862: def description(self, desc): # pylint: disable=E0202 On 2013/08/30 15:52:36, M-A Ruel wrote: > I must say I'm not a fan of keeping a setter here, it makes understand > foo.description = <something> much harder to understand and reduce the general > maintainability of this file. If someone wants to update the lines, > ._description_lines is there. > > Another option is an explicit set_description() member method. Yeah, you're right that the setter is confusing. I went the set_description() route, as it is very clear and I wanted to keep the functionality around. https://codereview.chromium.org/23072039/diff/12001/git_cl.py#newcode864 git_cl.py:864: lines = desc.strip().splitlines() On 2013/08/30 15:52:36, M-A Ruel wrote: > .strip() is not necessary because of lines 867-870. Done. https://codereview.chromium.org/23072039/diff/12001/git_cl.py#newcode866 git_cl.py:866: lines = [line.strip() for line in desc] On 2013/08/30 15:52:36, M-A Ruel wrote: > That's wrong, you are left-aligning everything. Done. https://codereview.chromium.org/23072039/diff/12001/git_cl.py#newcode868 git_cl.py:868: lines = lines[1:] On 2013/08/30 15:52:36, M-A Ruel wrote: > lines.pop(0) > will just the same faster. Done. https://codereview.chromium.org/23072039/diff/12001/git_cl.py#newcode870 git_cl.py:870: lines = lines.pop() On 2013/08/30 15:52:36, M-A Ruel wrote: > That's wrong since you are keeping the last line; you want: > lines.pop() Done.
mostly nits https://codereview.chromium.org/23072039/diff/19001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode851 git_cl.py:851: R_LINE = r'^[ \t]*(TBR|R)[ \t]*=[ \t]*(.*?)[ \t]*$' Optionally, presubmit_support.Change.TAG_LINE_RE could be used which is the generic form to catch all tags. That's what add_footer() uses. https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode881 git_cl.py:881: new_desc = [self._description_lines[i] for i in range(len(matches)) this would be slightly shorter: new_desc = [l for i, l in enumerate(self._description_lines) if not matches[i]] https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode889 git_cl.py:889: if match is None: if not match: https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode893 git_cl.py:893: if is_tbr: if match.group(1) == 'TBR': https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode897 git_cl.py:897: r_final_names = reviewers[:] I'd prefer for you to do it at the top of the file, right after the check, so it's more obvious that the input is not modified within the function. It's fine to keep the variable name reviewers and not alias it. https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode904 git_cl.py:904: # Put the new lines in the desciption where the old first R= line was. description https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode906 git_cl.py:906: if line_loc != -1 and line_loc < len(self._description_lines): if 0 <= line_loc < len(self._description_lines): https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode937 git_cl.py:937: clean_lines = [line.strip() for line in lines rstrip() https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode938 git_cl.py:938: if not re.match(r'^#.*$', line)] if not line.startswith('#')
https://codereview.chromium.org/23072039/diff/19001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode851 git_cl.py:851: R_LINE = r'^[ \t]*(TBR|R)[ \t]*=[ \t]*(.*?)[ \t]*$' On 2013/09/12 00:25:44, M-A Ruel wrote: > Optionally, presubmit_support.Change.TAG_LINE_RE could be used which is the > generic form to catch all tags. That's what add_footer() uses. Considered that. But since we're very explicitly only looking for R, TBR, and BUG lines here I decided to keep these as-is. https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode881 git_cl.py:881: new_desc = [self._description_lines[i] for i in range(len(matches)) On 2013/09/12 00:25:44, M-A Ruel wrote: > this would be slightly shorter: > new_desc = [l for i, l in enumerate(self._description_lines) if not matches[i]] Done. https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode889 git_cl.py:889: if match is None: On 2013/09/12 00:25:44, M-A Ruel wrote: > if not match: Done. https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode893 git_cl.py:893: if is_tbr: On 2013/09/12 00:25:44, M-A Ruel wrote: > if match.group(1) == 'TBR': Done. https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode897 git_cl.py:897: r_final_names = reviewers[:] On 2013/09/12 00:25:44, M-A Ruel wrote: > I'd prefer for you to do it at the top of the file, right after the check, so > it's more obvious that the input is not modified within the function. It's fine > to keep the variable name reviewers and not alias it. Done. https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode904 git_cl.py:904: # Put the new lines in the desciption where the old first R= line was. On 2013/09/12 00:25:44, M-A Ruel wrote: > description Done. https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode906 git_cl.py:906: if line_loc != -1 and line_loc < len(self._description_lines): On 2013/09/12 00:25:44, M-A Ruel wrote: > if 0 <= line_loc < len(self._description_lines): Done. https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode937 git_cl.py:937: clean_lines = [line.strip() for line in lines On 2013/09/12 00:25:44, M-A Ruel wrote: > rstrip() Done. https://codereview.chromium.org/23072039/diff/19001/git_cl.py#newcode938 git_cl.py:938: if not re.match(r'^#.*$', line)] On 2013/09/12 00:25:44, M-A Ruel wrote: > if not line.startswith('#') Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agable@chromium.org/23072039/24001
Message was sent while issue was closed.
Change committed as 222801 |
