Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12712002:
An interactive tool to help find owners covering current change list. (Closed)
|
Created:
7 years, 9 months ago by Bei Zhang Modified:
7 years, 3 months ago CC:
chromium-reviews, cmp-cc_chromium.org, iannucci+depot_tools_chromium.org Visibility:
Public. |
DescriptionAn interactive tool to help find owners covering current change list.
BUG=77248
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=224264
Patch Set 1 : Refactor the script to integrate with git cl #Patch Set 2 : Remove files that * can review from the process #
Total comments: 6
Patch Set 3 : More docs and minor improvements #
Total comments: 3
Patch Set 4 : Fixes and optimization #
Total comments: 22
Patch Set 5 : Adding unit tests #
Total comments: 16
Patch Set 6 : Fix Deselect #
Total comments: 18
Patch Set 7 : Fix style #
Total comments: 4
Patch Set 8 : Fixes #Patch Set 9 : Sort owners by relevance #Patch Set 10 : Fix bugs #
Total comments: 29
Patch Set 11 : Refactor scoring #
Total comments: 18
Patch Set 12 : Reuse scoring #
Total comments: 24
Patch Set 13 : Fix nits #
Total comments: 1
Messages
Total messages: 40 (0 generated)
Hi Vince, Please review. Thanks, Bei
You're basically duplicating the work done in the the CheckOwners() check in presubmit_canned_checks.py and the owners.py modules. I think I probably don't understand what you're trying to get out of this, but is what those routines produce not good enough? If not, we should at least be reusing more of the code. You definitely shouldn't be parsing the owner files yourself.
Hi Dirk, Thank you very much for your reply! This is an interactive tool to help you determine which owners you want to pick for the owners review (please try it). It is not generating the list automatically, but to provide the information needed for developers to determine their own pick. I need to parse the OWNERS files my self because I need the comment information which is ignored by the owners.py. Thanks, Bei On 2013/03/08 19:32:06, Dirk Pranke wrote: > You're basically duplicating the work done in the the CheckOwners() check in > presubmit_canned_checks.py and the owners.py modules. > > I think I probably don't understand what you're trying to get out of this, but > is what those routines produce not good enough? If not, we should at least be > reusing more of the code. You definitely shouldn't be parsing the owner files > yourself.
Hi Dirk, It is integrated into the git cl. Please review. Thanks, Bei
lgtm with some suggestions, though I suspect Dirk will have much better review of this code. https://codereview.chromium.org/12712002/diff/16001/find_owners.py File find_owners.py (right): https://codereview.chromium.org/12712002/diff/16001/find_owners.py#newcode25 find_owners.py:25: self.owners = owners = {} Perhaps comment what the expected structure of this object will be, e.g. dictionary of set of files. https://codereview.chromium.org/12712002/diff/16001/find_owners.py#newcode63 find_owners.py:63: owned_by = set() Maybe name this to be a set of files? https://codereview.chromium.org/12712002/diff/16001/find_owners.py#newcode189 find_owners.py:189: elif inp == "f" or inp == "files": Possibly factor out each of these sections into helper methods?
https://codereview.chromium.org/12712002/diff/16001/find_owners.py File find_owners.py (right): https://codereview.chromium.org/12712002/diff/16001/find_owners.py#newcode25 find_owners.py:25: self.owners = owners = {} On 2013/03/11 17:08:41, scheib wrote: > Perhaps comment what the expected structure of this object will be, e.g. > dictionary of set of files. Done. https://codereview.chromium.org/12712002/diff/16001/find_owners.py#newcode63 find_owners.py:63: owned_by = set() On 2013/03/11 17:08:41, scheib wrote: > Maybe name this to be a set of files? Done. https://codereview.chromium.org/12712002/diff/16001/find_owners.py#newcode189 find_owners.py:189: elif inp == "f" or inp == "files": On 2013/03/11 17:08:41, scheib wrote: > Possibly factor out each of these sections into helper methods? Done.
https://codereview.chromium.org/12712002/diff/19001/find_owners.py File find_owners.py (right): https://codereview.chromium.org/12712002/diff/19001/find_owners.py#newcode242 find_owners.py:242: inp = raw_input("[yes/no/Defer/pick/files/owners/quit]").lower() It's awkward to respond to the prompt with my inserting text adjacent to the closing ']'. Insert ': ' after the closing ']'. So the line is: inp = raw_input("[yes/no/Defer/pick/files/owners/quit]: ").lower()
https://codereview.chromium.org/12712002/diff/19001/find_owners.py File find_owners.py (right): https://codereview.chromium.org/12712002/diff/19001/find_owners.py#newcode242 find_owners.py:242: inp = raw_input("[yes/no/Defer/pick/files/owners/quit]").lower() On 2013/03/11 20:06:36, scheib wrote: > It's awkward to respond to the prompt with my inserting text adjacent to the > closing ']'. > > Insert ': ' after the closing ']'. So the line is: > inp = raw_input("[yes/no/Defer/pick/files/owners/quit]: ").lower() Done.
https://codereview.chromium.org/12712002/diff/19001/find_owners.py File find_owners.py (right): https://codereview.chromium.org/12712002/diff/19001/find_owners.py#newcode13 find_owners.py:13: import readline # pylint: disable=W0611 Not working on Windows
Generally speaking, this is much closer to something I'd be happy with. The changes to git_cl.py and owners.py look fine. find_owners.py needs some work, but it's not too far off. I've given you a bunch of comments but haven't looked at everything in detail yet, to avoid making you change too many things at once. One big problem is that we generally require unit tests for things like this, and your liberal use of 'print' and 'raw_input' is gonna make them kinda hard to write. You might want to refactor things so that you're returning strings where possible and/or use wrapper routines that you can mock out for unit tests. It's also hard to really understand what's going on in this code without some sort of samples to look at and tests are great for providing that sort of example. Take a look at the scaffolding I have in tests/owners_unittest.py for the type of things I'm talking about. https://codereview.chromium.org/12712002/diff/40002/find_owners.py File find_owners.py (right): https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode2 find_owners.py:2: # Copyright (c) 2011 The Chromium Authors. All rights reserved. 2013. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode15 find_owners.py:15: def first(iterable): I wouldn't use this function, it's easier to just say iterable[0] down below. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode20 find_owners.py:20: class FindOwners: class FindOwners(object): https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode25 find_owners.py:25: def __init__(self, files, local_root, disable_color=False): generally speaking, this method is too long, and it's bad still to do so much work in a constructor. Can you split this up? https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode54 find_owners.py:54: break can we just use db._all_possible_owners() here (you'd presumably through away the cost values of the tuples)? If not, we should change _all_possible_owners() to make this work, so that we're not duplicating this loop. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode74 find_owners.py:74: owner_to_files[owner_name] = files_set it's unfortunate that we can't use just db.owned_by here. Can you pull this loop into a separate function? (Maybe we can eventually move it into owners.py). https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode114 find_owners.py:114: def findMustPickOwners(self): so "findMustPickOwners" selects all the people that are the only option for a particular file in the list? I'd rename this to findMandatoryOwners and/or add a docstring. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode122 find_owners.py:122: break Can you rewrite this as a loop over a filtered list of files, e.g.: for filename in filter(lambda filename: len(self.file_to_owners[filename] == 1, self.unreviewedFiles): self.useOwner(self.file_to_owners[filename][0]) https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode222 find_owners.py:222: def runOnce(self): Can you document what runOnce() and run() are doing? I'm guessing that runOnce is handling one particular input from the user, but these names are not giving me very many clues. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode241 find_owners.py:241: # If this owner is already deseleted. deselected? https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode300 find_owners.py:300: def main(): Do you actually imagine this being used as a separate standalone script as well as part of git-cl ? I'd probably delete lines 300- just to simplify the number of things you need to support.
Thank you so much Dirk! I will refactor the code for unit tests. https://codereview.chromium.org/12712002/diff/40002/find_owners.py File find_owners.py (right): https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode2 find_owners.py:2: # Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2013/04/03 00:54:16, Dirk Pranke wrote: > 2013. Done. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode15 find_owners.py:15: def first(iterable): It's for unordered set though. Maybe I should use iterable.__iter__().next()? On 2013/04/03 00:54:16, Dirk Pranke wrote: > I wouldn't use this function, it's easier to just say iterable[0] down below. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode20 find_owners.py:20: class FindOwners: On 2013/04/03 00:54:16, Dirk Pranke wrote: > class FindOwners(object): Done. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode25 find_owners.py:25: def __init__(self, files, local_root, disable_color=False): On 2013/04/03 00:54:16, Dirk Pranke wrote: > generally speaking, this method is too long, and it's bad still to do so much > work in a constructor. Can you split this up? Done. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode54 find_owners.py:54: break I was a little bit scared of the leading underscore here. I'm not familiar with the language though (as you may already notice), maybe it's okay to call them? Also, to make _all_possible_owners work, we also need to call _check_paths, _load_data_needed_for and _enclosing_dir_with_owners methods. On 2013/04/03 00:54:16, Dirk Pranke wrote: > can we just use db._all_possible_owners() here (you'd presumably through away > the cost values of the tuples)? If not, we should change _all_possible_owners() > to make this work, so that we're not duplicating this loop. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode114 find_owners.py:114: def findMustPickOwners(self): Done. Sorry for my bad English. On 2013/04/03 00:54:16, Dirk Pranke wrote: > so "findMustPickOwners" selects all the people that are the only option for a > particular file in the list? > > I'd rename this to findMandatoryOwners and/or add a docstring. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode122 find_owners.py:122: break On 2013/04/03 00:54:16, Dirk Pranke wrote: > Can you rewrite this as a loop over a filtered list of files, e.g.: > > for filename in filter(lambda filename: len(self.file_to_owners[filename] == 1, > self.unreviewedFiles): > self.useOwner(self.file_to_owners[filename][0]) Done. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode222 find_owners.py:222: def runOnce(self): Done. It was for a hack to restart the run(). Removed. On 2013/04/03 00:54:16, Dirk Pranke wrote: > Can you document what runOnce() and run() are doing? I'm guessing that runOnce > is handling one particular input from the user, but these names are not giving > me very many clues. https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode241 find_owners.py:241: # If this owner is already deseleted. That is, when you select "no" on this owner. So every owner has 3 statuses: selected, deselected and deferred. You can defer a decision and the owner will be shown again at the end of the queue. On 2013/04/03 00:54:16, Dirk Pranke wrote: > deselected? https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode300 find_owners.py:300: def main(): On 2013/04/03 00:54:16, Dirk Pranke wrote: > Do you actually imagine this being used as a separate standalone script as well > as part of git-cl ? I'd probably delete lines 300- just to simplify the number > of things you need to support. Done.
Hi Dirk, Thank you for reviewing! Sorry I changed the name of "find_owners.py" to "owners_finder.py" and I think it failed to make a connection between the two. Can you review it without the diff? Thanks, Bei https://codereview.chromium.org/12712002/diff/40002/find_owners.py File find_owners.py (right): https://codereview.chromium.org/12712002/diff/40002/find_owners.py#newcode74 find_owners.py:74: owner_to_files[owner_name] = files_set On 2013/04/03 00:54:16, Dirk Pranke wrote: > it's unfortunate that we can't use just db.owned_by here. Can you pull this loop > into a separate function? (Maybe we can eventually move it into owners.py). Done.
a few nits https://codereview.chromium.org/12712002/diff/47001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/12712002/diff/47001/git_cl.py#newcode38 git_cl.py:38: another line https://codereview.chromium.org/12712002/diff/47001/git_cl.py#newcode1896 git_cl.py:1896: 2 lines https://codereview.chromium.org/12712002/diff/47001/git_cl.py#newcode1899 git_cl.py:1899: group = optparse.OptionGroup(parser, "Find owners options") Use ' instead of " https://codereview.chromium.org/12712002/diff/47001/git_cl.py#newcode1901 git_cl.py:1901: "--no-color", dest="ncolor", default=False, no need to default=False I'd prefer to not use dest and just use the default name. https://codereview.chromium.org/12712002/diff/47001/git_cl.py#newcode1907 git_cl.py:1907: if len(args) > 1: parser.error('Unknown args https://codereview.chromium.org/12712002/diff/47001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/47001/owners_finder.py#newcode1 owners_finder.py:1: #!/usr/bin/python Remove, this file is not executable. https://codereview.chromium.org/12712002/diff/47001/owners_finder.py#newcode6 owners_finder.py:6: """A tool to help picking owner_to_files for reviewing.""" """Helps picking .. https://codereview.chromium.org/12712002/diff/47001/tests/owners_finder_test.py File tests/owners_finder_test.py (right): https://codereview.chromium.org/12712002/diff/47001/tests/owners_finder_test.... tests/owners_finder_test.py:133: self.assertEquals(finder.selected_owners, set()) assertEquals is deprecated, use assertEqual.
https://codereview.chromium.org/12712002/diff/47001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/12712002/diff/47001/git_cl.py#newcode38 git_cl.py:38: On 2013/04/24 01:32:20, Marc-Antoine Ruel wrote: > another line Done. https://codereview.chromium.org/12712002/diff/47001/git_cl.py#newcode1896 git_cl.py:1896: On 2013/04/24 01:32:20, Marc-Antoine Ruel wrote: > 2 lines Done. https://codereview.chromium.org/12712002/diff/47001/git_cl.py#newcode1899 git_cl.py:1899: group = optparse.OptionGroup(parser, "Find owners options") On 2013/04/24 01:32:20, Marc-Antoine Ruel wrote: > Use ' instead of " Done. https://codereview.chromium.org/12712002/diff/47001/git_cl.py#newcode1901 git_cl.py:1901: "--no-color", dest="ncolor", default=False, On 2013/04/24 01:32:20, Marc-Antoine Ruel wrote: > no need to default=False > I'd prefer to not use dest and just use the default name. Done. https://codereview.chromium.org/12712002/diff/47001/git_cl.py#newcode1907 git_cl.py:1907: On 2013/04/24 01:32:20, Marc-Antoine Ruel wrote: > if len(args) > 1: > parser.error('Unknown args Done. https://codereview.chromium.org/12712002/diff/47001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/47001/owners_finder.py#newcode1 owners_finder.py:1: #!/usr/bin/python On 2013/04/24 01:32:20, Marc-Antoine Ruel wrote: > Remove, this file is not executable. Done. https://codereview.chromium.org/12712002/diff/47001/owners_finder.py#newcode6 owners_finder.py:6: """A tool to help picking owner_to_files for reviewing.""" On 2013/04/24 01:32:20, Marc-Antoine Ruel wrote: > """Helps picking .. Done. https://codereview.chromium.org/12712002/diff/47001/tests/owners_finder_test.py File tests/owners_finder_test.py (right): https://codereview.chromium.org/12712002/diff/47001/tests/owners_finder_test.... tests/owners_finder_test.py:133: self.assertEquals(finder.selected_owners, set()) On 2013/04/24 01:32:20, Marc-Antoine Ruel wrote: > assertEquals is deprecated, use assertEqual. Done.
https://codereview.chromium.org/12712002/diff/62014/git_cl.py File git_cl.py (right): https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode37 git_cl.py:37: import glob this one is a stdlib, move it in the group above. https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode1900 git_cl.py:1900: """Interactively find the owners for reviewing""" """interactively ... because it's printed as-is in git cl help. https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode1905 git_cl.py:1905: help=('Use this option to disable color output')) help='Use this option to disable color output') https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode1907 git_cl.py:1907: opt, args = parser.parse_args(args) please use the same naming conversion than the others; options, args = parser.parse_args(args) https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode1911 git_cl.py:1911: if len(args) > 1: Do this check first, before creating cl. https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode1913 git_cl.py:1913: return not needed, remove. error() calls sys.exit() https://codereview.chromium.org/12712002/diff/62014/owners.py File owners.py (right): https://codereview.chromium.org/12712002/diff/62014/owners.py#newcode201 owners.py:201: comment = "" '' everywhere https://codereview.chromium.org/12712002/diff/62014/owners.py#newcode202 owners.py:202: last_line_is_comment = False This is not needed. comments = [] Then accumulate the comment lines with: comments.append(line[1:].strip()) at lines 215 and 235, add comments = [] that's all https://codereview.chromium.org/12712002/diff/62014/owners.py#newcode209 owners.py:209: comment += " " + line[1:].strip() why not keep the LF as-is?
Thanks for the review! https://codereview.chromium.org/12712002/diff/62014/git_cl.py File git_cl.py (right): https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode37 git_cl.py:37: import glob On 2013/04/24 13:04:45, Marc-Antoine Ruel wrote: > this one is a stdlib, move it in the group above. Done. https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode1900 git_cl.py:1900: """Interactively find the owners for reviewing""" On 2013/04/24 13:04:45, Marc-Antoine Ruel wrote: > """interactively ... > > because it's printed as-is in git cl help. Done. https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode1905 git_cl.py:1905: help=('Use this option to disable color output')) On 2013/04/24 13:04:45, Marc-Antoine Ruel wrote: > help='Use this option to disable color output') Done. https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode1907 git_cl.py:1907: opt, args = parser.parse_args(args) On 2013/04/24 13:04:45, Marc-Antoine Ruel wrote: > please use the same naming conversion than the others; > options, args = parser.parse_args(args) Done. https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode1911 git_cl.py:1911: if len(args) > 1: On 2013/04/24 13:04:45, Marc-Antoine Ruel wrote: > Do this check first, before creating cl. Done. https://codereview.chromium.org/12712002/diff/62014/git_cl.py#newcode1913 git_cl.py:1913: return On 2013/04/24 13:04:45, Marc-Antoine Ruel wrote: > not needed, remove. error() calls sys.exit() Done. https://codereview.chromium.org/12712002/diff/62014/owners.py File owners.py (right): https://codereview.chromium.org/12712002/diff/62014/owners.py#newcode201 owners.py:201: comment = "" On 2013/04/24 13:04:45, Marc-Antoine Ruel wrote: > '' everywhere Done. https://codereview.chromium.org/12712002/diff/62014/owners.py#newcode202 owners.py:202: last_line_is_comment = False That case I will have to introduce a last_comment variable because I cannot clear it right away. It is referenced at line 233 in probably another iteration. On 2013/04/24 13:04:45, Marc-Antoine Ruel wrote: > This is not needed. > > comments = [] > Then accumulate the comment lines with: > comments.append(line[1:].strip()) > at lines 215 and 235, add > comments = [] > > that's all https://codereview.chromium.org/12712002/diff/62014/owners.py#newcode209 owners.py:209: comment += " " + line[1:].strip() I think the comment could be wrapped. Do you think I should keep them as is? On 2013/04/24 13:04:45, Marc-Antoine Ruel wrote: > why not keep the LF as-is?
https://codereview.chromium.org/12712002/diff/70002/owners.py File owners.py (right): https://codereview.chromium.org/12712002/diff/70002/owners.py#newcode208 owners.py:208: if last_line_is_comment: if you were using comments as a list, you would just use ' '.join(comments) and wouldn't need last_line_is_comment at all. I don't understand why you would need last_comment, line 234 would have right after usage: comments = [] Same for line 215. https://codereview.chromium.org/12712002/diff/70002/owners.py#newcode248 owners.py:248: if directive not in self.comments: self.comments.setdefault(directive, {})
https://codereview.chromium.org/12712002/diff/70002/owners.py File owners.py (right): https://codereview.chromium.org/12712002/diff/70002/owners.py#newcode208 owners.py:208: if last_line_is_comment: I agree that we should use a list instead of string. About the last_comment, please consider this example: ================================ # Comment 1... # and 2 goes to darin and peter. darin@chromium.org peter@chromium.org # Comment 3 goes to ben. ben@chromium.org ================================ How do I know where to start a new comment? We cannot clear the comments right after usage because that will miss all the following owners sharing the same comment. We could either: 1. start a new comment when parsing new comments if the last line is not a comment; 2. or clear the comment right away after saving the comment into another variable as a last comment; meanwhile clear the last comment variable when parsing new comments. Either way, we'll need extra information to split the comments. On 2013/04/24 18:43:45, Marc-Antoine Ruel wrote: > if you were using comments as a list, you would just use ' '.join(comments) and > wouldn't need last_line_is_comment at all. > > I don't understand why you would need last_comment, line 234 would have right > after usage: > comments = [] > Same for line 215. https://codereview.chromium.org/12712002/diff/70002/owners.py#newcode248 owners.py:248: if directive not in self.comments: On 2013/04/24 18:43:45, Marc-Antoine Ruel wrote: > self.comments.setdefault(directive, {}) Thanks. This looks much better.
This patch will address https://code.google.com/p/chromium/issues/detail?id=77248 add to BUG
Hi Gentlemen, Just a friendly ping here. Do you have time to review this CL? Thanks, Bei
On 2013/07/22 22:23:42, Bei Zhang wrote: > Hi Gentlemen, > > Just a friendly ping here. Do you have time to review this CL? > > Thanks, > Bei I'll let Dirk do the bulk of the review relating to OWNERS check.
Hi Bei, I know this has been hanging out in my queue forever :(. I'll try again to get to it either later today or tomorrow. Sorry! -- Dirk On Mon, Jul 22, 2013 at 3:23 PM, <ikarienator@chromium.org> wrote: > Hi Gentlemen, > > Just a friendly ping here. Do you have time to review this CL? > > Thanks, > Bei > > > https://codereview.chromium.**org/12712002/<https://codereview.chromium.org/1... >
Sorry for the horrendous delay in reviewing this. Here's some initial comments. I haven't sweated the details of owners_finder yet, as I want to make sure I understand the high-level approach first. I also haven't looked at the tests yet, so let me know if my comments about restructuring the code to make it easier to test are way off base. https://codereview.chromium.org/12712002/diff/100001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/12712002/diff/100001/git_cl.py#newcode2011 git_cl.py:2011: parser.add_option_group(group) I probably wouldn't use an OptionGroup for this, just add the options directly to the parser. https://codereview.chromium.org/12712002/diff/100001/owners.py File owners.py (right): https://codereview.chromium.org/12712002/diff/100001/owners.py#newcode114 owners.py:114: # Mapping reviewers to the most recent comment in the OWNERS files. I might change "most recent" to "preceding" here ... Also this looks like this is actually a map of person -> {path -> comment}, not person -> comment https://codereview.chromium.org/12712002/diff/100001/owners.py#newcode119 owners.py:119: self.stop_looking = {''} We still have to support 2.6, so we can't use the set literal syntax yet. https://codereview.chromium.org/12712002/diff/100001/owners.py#newcode134 owners.py:134: suggested_owners = {'<anyone>'} same comment. https://codereview.chromium.org/12712002/diff/100001/owners.py#newcode202 owners.py:202: last_line_is_comment = False nits: I might call this 'current_comment' and 'in_comment' or 'previous_line_was_comment', but these aren't huge improvements. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode5 owners_finder.py:5: """Helps picking owner_to_files for reviewing.""" """Interactive tool for finding reviewers/owners for a change.""" ? Also, there should be some documentation for this somewhere; perhaps here, or in the help text for CMDowners, or on dev.chromium.org or ... It would be good to have both user documentation and some internal documentation over the structure of the algorithm. The latter could be done here or in a docstring for OwnersFinder. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode8 owners_finder.py:8: import copy You should probably pass these references to the __init__() rather than importing them here. That'll make things easier to mock-out and test. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode41 owners_finder.py:41: self.owners_score = {} Add some comments to say what the owners_score and owners_queue for ? https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode58 owners_finder.py:58: return db Is there a reason not to just inline this into __init__() ? https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode60 owners_finder.py:60: def _map_files_to_owners(self, files, db): You should add some comments about what this routine is doing ... what is the owners_score data structure for? How is the logic here different from the logic in owners.py's lowest_cost_owner() ? Can we re-use some of the code ? https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode118 owners_finder.py:118: def bold(self, text): It seems like the only public method is really run(). I would perhaps restructure this file so that run() follows init() and then the other methods follow, top-down (or in some similar order) so that a new reader can get a sense of how things work by just reading linearly through the file. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode327 owners_finder.py:327: owner = self.owners_queue[0] should this be owner = self.owners_queue.pop(0) ? It's not clear to me that this is actually a queue since it doesn't look like you pop things in the order they show up in the list. Does the code become clearer if you actually remove the owners one at a time and deal with them (re-adding if necessary when they're deferred)? (I'm not actually sure if this is the right thing to do as I don't fully understand the algorithm yet, but I think that's part of the problem as the name suggests to me things work a particular way and I'm not sure if that's true). https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode344 owners_finder.py:344: self.print_owned_files_for(owner) I'd probably pull lines 341-344 into a separate method just to provide a little better separation between the logic and the interface. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode349 owners_finder.py:349: '[yes/no/Defer/pick/files/owners/quit/restart]: ').lower() Calling raw_input() directly makes this hard to write unit tests ... I'd split lines 347-349 out into a separate method that also validates and canonicalizes the input. That should make the rest of this loop more concise.
https://codereview.chromium.org/12712002/diff/100001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/12712002/diff/100001/git_cl.py#newcode2011 git_cl.py:2011: parser.add_option_group(group) On 2013/07/27 00:06:03, Dirk Pranke wrote: > I probably wouldn't use an OptionGroup for this, just add the options directly > to the parser. Done. https://codereview.chromium.org/12712002/diff/100001/owners.py File owners.py (right): https://codereview.chromium.org/12712002/diff/100001/owners.py#newcode114 owners.py:114: # Mapping reviewers to the most recent comment in the OWNERS files. On 2013/07/27 00:06:03, Dirk Pranke wrote: > I might change "most recent" to "preceding" here ... > > Also this looks like this is actually a map of person -> {path -> comment}, not > person -> comment Done. https://codereview.chromium.org/12712002/diff/100001/owners.py#newcode119 owners.py:119: self.stop_looking = {''} On 2013/07/27 00:06:03, Dirk Pranke wrote: > We still have to support 2.6, so we can't use the set literal syntax yet. Done. https://codereview.chromium.org/12712002/diff/100001/owners.py#newcode134 owners.py:134: suggested_owners = {'<anyone>'} On 2013/07/27 00:06:03, Dirk Pranke wrote: > same comment. Done. https://codereview.chromium.org/12712002/diff/100001/owners.py#newcode202 owners.py:202: last_line_is_comment = False On 2013/07/27 00:06:03, Dirk Pranke wrote: > nits: I might call this 'current_comment' and 'in_comment' or > 'previous_line_was_comment', but these aren't huge improvements. Done. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode5 owners_finder.py:5: """Helps picking owner_to_files for reviewing.""" I would like to put it on dev.chromium.org. How do I do that? On 2013/07/27 00:06:03, Dirk Pranke wrote: > """Interactive tool for finding reviewers/owners for a change.""" ? > > Also, there should be some documentation for this somewhere; perhaps here, or in > the help text for CMDowners, or on http://dev.chromium.org or ... > > It would be good to have both user documentation and some internal documentation > over the structure of the algorithm. The latter could be done here or in a > docstring for OwnersFinder. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode8 owners_finder.py:8: import copy On 2013/07/27 00:06:03, Dirk Pranke wrote: > You should probably pass these references to the __init__() rather than > importing them here. That'll make things easier to mock-out and test. Done. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode41 owners_finder.py:41: self.owners_score = {} On 2013/07/27 00:06:03, Dirk Pranke wrote: > Add some comments to say what the owners_score and owners_queue for ? Done. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode58 owners_finder.py:58: return db On 2013/07/27 00:06:03, Dirk Pranke wrote: > Is there a reason not to just inline this into __init__() ? Done. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode60 owners_finder.py:60: def _map_files_to_owners(self, files, db): I extracted the scoring routing. I think it is possible to reuse the scoring in Database but they work somehow differently. So I think I probably will do that in another CL. On 2013/07/27 00:06:03, Dirk Pranke wrote: > You should add some comments about what this routine is doing ... what is the > owners_score data structure for? How is the logic here different from the logic > in owners.py's lowest_cost_owner() ? Can we re-use some of the code ? https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode118 owners_finder.py:118: def bold(self, text): On 2013/07/27 00:06:03, Dirk Pranke wrote: > It seems like the only public method is really run(). I would perhaps > restructure this file so that run() follows init() and then the other methods > follow, top-down (or in some similar order) so that a new reader can get a sense > of how things work by just reading linearly through the file. Done. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode327 owners_finder.py:327: owner = self.owners_queue[0] There are several decisions have to be made according to the full list of the owners_queue. So I defer the pop later. It is a queue because things are always inserted at the tail of the queue, and the current owner is always get from the head of the queue. On 2013/07/27 00:06:03, Dirk Pranke wrote: > should this be owner = self.owners_queue.pop(0) ? > > It's not clear to me that this is actually a queue since it doesn't look like > you pop things in the order they show up in the list. Does the code become > clearer if you actually remove the owners one at a time and deal with them > (re-adding if necessary when they're deferred)? > > (I'm not actually sure if this is the right thing to do as I don't fully > understand the algorithm yet, but I think that's part of the problem as the name > suggests to me things work a particular way and I'm not sure if that's true). https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode344 owners_finder.py:344: self.print_owned_files_for(owner) On 2013/07/27 00:06:03, Dirk Pranke wrote: > I'd probably pull lines 341-344 into a separate method just to provide a little > better separation between the logic and the interface. Done. https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode349 owners_finder.py:349: '[yes/no/Defer/pick/files/owners/quit/restart]: ').lower() On 2013/07/27 00:06:03, Dirk Pranke wrote: > Calling raw_input() directly makes this hard to write unit tests ... > > I'd split lines 347-349 out into a separate method that also validates and > canonicalizes the input. That should make the rest of this loop more concise. Done.
https://codereview.chromium.org/12712002/diff/100001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/100001/owners_finder.py#newcode327 owners_finder.py:327: owner = self.owners_queue[0] On 2013/07/30 05:59:17, Bei Zhang wrote: > There are several decisions have to be made according to the full list of the > owners_queue. So I defer the pop later. It is a queue because things are always > inserted at the tail of the queue, and the current owner is always get from the > head of the queue. > I don't think I'm seeing the decisions you have in mind. Can you point them out to me? In particular, it seems like if you did a pop up front, you could get rid of the 'if .. continue' blocks on lines 329-330 and 334-336. It also seems like you could simplify select_owner() and deselect_owner() quite a bit, and then the remove()/pop() call would be in the same place as where you are iterating over the list. All of this feels like it would be clearer to me (but perhaps it doesn't work for reasons I'm missing ...). https://codereview.chromium.org/12712002/diff/107001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/12712002/diff/107001/git_cl.py#newcode24 git_cl.py:24: import glob Nit: sort the imports so that this is up by json. https://codereview.chromium.org/12712002/diff/107001/git_cl.py#newcode2097 git_cl.py:2097: base_branch = RunGit(['merge-base', cl.GetUpstreamBranch(), 'HEAD']).strip() What happens if we try to run this on an svn checkout? https://codereview.chromium.org/12712002/diff/107001/owners.py File owners.py (right): https://codereview.chromium.org/12712002/diff/107001/owners.py#newcode209 owners.py:209: comment = [] Can you just move "comment = []" next to line 215 and get rid of line 208? https://codereview.chromium.org/12712002/diff/107001/owners.py#newcode233 owners.py:233: owners_path, lineno, ' '.join(comment)) Do you really want to join with ' ' and not '\n' ? https://codereview.chromium.org/12712002/diff/107001/owners.py#newcode308 owners.py:308: pow(num_directories_owned, 1.75)) Is this change intentional? I.e., did you find a bug in how I was computing the total costs_by_owner? It looks like I was just doing the computation multiple times unnecessarily, but maybe I'm missing something ... https://codereview.chromium.org/12712002/diff/107001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/107001/owners_finder.py#newcode43 owners_finder.py:43: self._map_files_to_owners(files) Can you use db.owned_by and db.owners_for here and delete _map_files_to_owners(), _owners_of(), and map_owner_to_files()? It feels like you're duplicating work being done in db.load_data_needed_for(). https://codereview.chromium.org/12712002/diff/107001/owners_finder.py#newcode59 owners_finder.py:59: # new entry. I think there's a typo in this sentence. https://codereview.chromium.org/12712002/diff/107001/owners_finder.py#newcode64 owners_finder.py:64: # score from that file. I still don't understand how you came up with this seemingly arbitrary way to order/score owners and why it should be different from the seemingly arbitrary way to order/score owners we have in owners.py. We should only have one seemingly arbitrary way to do such things. Did you find orderings that were bad with one and better with the other? Is there some reason we'd want scores to be different? https://codereview.chromium.org/12712002/diff/107001/owners_finder.py#newcode175 owners_finder.py:175: # Files that EVERYONE owns is already eliminated. Nit: "are" already eliminated. https://codereview.chromium.org/12712002/diff/107001/owners_finder.py#newcode181 owners_finder.py:181: pow(0.1, depth) / len(self.db.owners_for[entry_name]) Same comments as above re: scoring ...
Coming back to this patch after a bit ... I'm sorry it's been such a hassle getting this committed for you. It is pretty cool functionality. I should make it clear that I'm not particularly wedded to the existing functionality in owners.py. It's mostly that the existing code is already kind of an obscure bunch of logic, and I don't really want to have two sets of obscure logic that kind of do similar things, but not really. So, one way to move forward would be to rip out the _covering_set_of_owners() code in owners.py and rewrite it to use your algorithm and just act as if we used the defaults at every step (so that it was non-interactive). Does that work for you?
Hi Dirk, I've started working on reuse the scoring algorithm in owners.py, this will get the code in faster. To me, it look like reusing my scoring algorithm in owners.py should be done in separate CL. So I will try to get this in and then think about other options. I have some other ideas on improving the current queuing algorithm, but it also deserves a separate CL. Thanks, Bei On 2013/08/10 00:22:25, Dirk Pranke wrote: > Coming back to this patch after a bit ... I'm sorry it's been such a hassle > getting this committed for you. It is pretty cool functionality. > > I should make it clear that I'm not particularly wedded to the existing > functionality in owners.py. It's mostly that the existing code is already kind > of an obscure bunch of logic, and I don't really want to have two sets of > obscure logic that kind of do similar things, but not really. > > So, one way to move forward would be to rip out the _covering_set_of_owners() > code in owners.py and rewrite it to use your algorithm and just act as if we > used the defaults at every step (so that it was non-interactive). > > Does that work for you?
https://codereview.chromium.org/12712002/diff/107001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/12712002/diff/107001/git_cl.py#newcode24 git_cl.py:24: import glob On 2013/07/30 22:01:00, Dirk Pranke wrote: > Nit: sort the imports so that this is up by json. Done. https://codereview.chromium.org/12712002/diff/107001/git_cl.py#newcode2097 git_cl.py:2097: base_branch = RunGit(['merge-base', cl.GetUpstreamBranch(), 'HEAD']).strip() This part is copied from CMDpresubmit. So I guess it probably works with git-svn? On 2013/07/30 22:01:00, Dirk Pranke wrote: > What happens if we try to run this on an svn checkout? https://codereview.chromium.org/12712002/diff/107001/owners.py File owners.py (right): https://codereview.chromium.org/12712002/diff/107001/owners.py#newcode209 owners.py:209: comment = [] No, you can't. That will prevent comment sharing. On 2013/07/30 22:01:00, Dirk Pranke wrote: > Can you just move "comment = []" next to line 215 and get rid of line 208? https://codereview.chromium.org/12712002/diff/107001/owners.py#newcode233 owners.py:233: owners_path, lineno, ' '.join(comment)) Good idea. So we can wrap exactly as was in OWNERS. On 2013/07/30 22:01:00, Dirk Pranke wrote: > Do you really want to join with ' ' and not '\n' ? https://codereview.chromium.org/12712002/diff/107001/owners.py#newcode308 owners.py:308: pow(num_directories_owned, 1.75)) Yes this is just an optimization. On 2013/07/30 22:01:00, Dirk Pranke wrote: > Is this change intentional? I.e., did you find a bug in how I was computing the > total costs_by_owner? It looks like I was just doing the computation multiple > times unnecessarily, but maybe I'm missing something ... https://codereview.chromium.org/12712002/diff/107001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/107001/owners_finder.py#newcode43 owners_finder.py:43: self._map_files_to_owners(files) They're for different purposes. db.owned_by and db.owners_for will contain entries that are not included in the change list, e.g. the ancestor dirs of changed files, and files mentioned in per-file action in those dirs. On 2013/07/30 22:01:00, Dirk Pranke wrote: > Can you use db.owned_by and db.owners_for here and delete > _map_files_to_owners(), _owners_of(), and map_owner_to_files()? It feels like > you're duplicating work being done in db.load_data_needed_for(). https://codereview.chromium.org/12712002/diff/107001/owners_finder.py#newcode59 owners_finder.py:59: # new entry. On 2013/07/30 22:01:00, Dirk Pranke wrote: > I think there's a typo in this sentence. Done. https://codereview.chromium.org/12712002/diff/107001/owners_finder.py#newcode64 owners_finder.py:64: # score from that file. There is no reason. I will try to reuse it. On 2013/07/30 22:01:00, Dirk Pranke wrote: > I still don't understand how you came up with this seemingly arbitrary way to > order/score owners and why it should be different from the seemingly arbitrary > way to order/score owners we have in owners.py. > > We should only have one seemingly arbitrary way to do such things. > > Did you find orderings that were bad with one and better with the other? Is > there some reason we'd want scores to be different?
... really looking forward to this patch ... maruel, dpranke, can you take another look?
https://codereview.chromium.org/12712002/diff/126001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/12712002/diff/126001/git_cl.py#newcode2103 git_cl.py:2103: cl = Changelist() Move it above line 2100, so you can remove this one and line 2106. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode18 owners_finder.py:18: COLOR_LINK = '\033[4m' Use colorama, it works on Windows.
maruel, these sounds like we're down to nits, other than these does the patch get an l g t m? Let's reduce iteration time here. On Mon, Sep 16, 2013 at 1:33 PM, <maruel@chromium.org> wrote: > > https://codereview.chromium.**org/12712002/diff/126001/git_**cl.py<https://co... > File git_cl.py (right): > > https://codereview.chromium.**org/12712002/diff/126001/git_** > cl.py#newcode2103<https://codereview.chromium.org/12712002/diff/126001/git_cl.py#newcode2103> > git_cl.py:2103: cl = Changelist() > Move it above line 2100, so you can remove this one and line 2106. > > https://codereview.chromium.**org/12712002/diff/126001/**owners_finder.py<htt... > File owners_finder.py (right): > > https://codereview.chromium.**org/12712002/diff/126001/** > owners_finder.py#newcode18<https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode18> > owners_finder.py:18: COLOR_LINK = '\033[4m' > Use colorama, it works on Windows. > > https://codereview.chromium.**org/12712002/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm sorry, This keeps falling off my radar. I will try to take another look this afternoon. Don't pick on M-A :). On Mon, Sep 16, 2013 at 1:19 PM, <scheib@chromium.org> wrote: > ... really looking forward to this patch ... maruel, dpranke, can you take > another look? > > https://codereview.chromium.**org/12712002/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
This basically looks okay, and I think we're getting close. I think there's one outright bug, and a couple of clarifying points to make sure I understand what's going on. The rest are just suggestions. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode65 owners_finder.py:65: files = eliminated_files This is confusing ... if I'm reading this correctly, if files were eliminated, then don't we reset things so that we're *only* looking at the eliminated files? Don't you want to set files to the files that *were not* eliminated? https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode98 owners_finder.py:98: while len(self.owners_queue) > 0 and len(self.unreviewed_files) > 0: Nit: this could be just: while len(self.owners_queue) and len(self.unreviewed_files): or even: while self.owners_queue and self.unreviewed_files: https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode106 owners_finder.py:106: break Will lines 104-106 ever execute, given the check on line 98 ? https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode109 owners_finder.py:109: continue Nit: can you merge lines 107-109 with lines 101-102? https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode156 owners_finder.py:156: self.owners_to_files[owner].add(file_name) Is this different from self.owners.owned_by ? https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode162 owners_finder.py:162: self.files_to_owners[file_name].add(owner) Is this different from self.owners.owners_for ? Are these two tracking the current/proposed mapping? If so, maybe that should be reflected in the names? https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode182 owners_finder.py:182: return Nit: I'd probably collapse these into one 'if' block. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode200 owners_finder.py:200: return Nit: ditto. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode227 owners_finder.py:227: break I think you can re-work lines 219-227 to: single_owner_files = [file_name for file_name in self.unreviewed_files if len(self.files_to_owners[file_name]) == 1] mandatory_owners = set(files_to_owners([file_name][0] for file_name in single_owner_files]) for owner in mandatory_owners: self.select_owner(owner, False) I think your code has the problem that if multiple files are owned by the same single owner, you will try to select the owner multiple times. Which is probably harmless, but perhaps not as clear as the above. Line 3 in my suggestion is probably also the same result what 'self.owners.reviewers_for(single_owner_files)' would return, so that might also be a clearer change.
https://codereview.chromium.org/12712002/diff/126001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/12712002/diff/126001/git_cl.py#newcode2103 git_cl.py:2103: cl = Changelist() On 2013/09/16 20:33:13, M-A Ruel wrote: > Move it above line 2100, so you can remove this one and line 2106. Done. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode18 owners_finder.py:18: COLOR_LINK = '\033[4m' On 2013/09/16 20:33:13, M-A Ruel wrote: > Use colorama, it works on Windows. Is that installed by default? I will probably address this problem in an additional CL. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode65 owners_finder.py:65: files = eliminated_files This should be named "filterd_files". On 2013/09/16 23:56:11, Dirk Pranke wrote: > This is confusing ... if I'm reading this correctly, if files were eliminated, > then don't we reset things so that we're *only* looking at the eliminated files? > > Don't you want to set files to the files that *were not* eliminated? https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode98 owners_finder.py:98: while len(self.owners_queue) > 0 and len(self.unreviewed_files) > 0: On 2013/09/16 23:56:11, Dirk Pranke wrote: > Nit: this could be just: > > while len(self.owners_queue) and len(self.unreviewed_files): > > or even: > > while self.owners_queue and self.unreviewed_files: > Done. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode106 owners_finder.py:106: break On 2013/09/16 23:56:11, Dirk Pranke wrote: > Will lines 104-106 ever execute, given the check on line 98 ? Done. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode109 owners_finder.py:109: continue On 2013/09/16 23:56:11, Dirk Pranke wrote: > Nit: can you merge lines 107-109 with lines 101-102? Done. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode156 owners_finder.py:156: self.owners_to_files[owner].add(file_name) There is no self.owners.owned_by. There is a self.db.owned_by which includes all the files mentioned in the OWNERS including those are not in the CL. On 2013/09/16 23:56:11, Dirk Pranke wrote: > Is this different from self.owners.owned_by ? https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode162 owners_finder.py:162: self.files_to_owners[file_name].add(owner) Likewise. Only interesting files are included. On 2013/09/16 23:56:11, Dirk Pranke wrote: > Is this different from self.owners.owners_for ? > > Are these two tracking the current/proposed mapping? If so, maybe that should be > reflected in the names? https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode182 owners_finder.py:182: return On 2013/09/16 23:56:11, Dirk Pranke wrote: > Nit: I'd probably collapse these into one 'if' block. Done. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode200 owners_finder.py:200: return On 2013/09/16 23:56:11, Dirk Pranke wrote: > Nit: ditto. Done. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode227 owners_finder.py:227: break On 2013/09/16 23:56:11, Dirk Pranke wrote: > I think you can re-work lines 219-227 to: > > single_owner_files = [file_name for file_name in self.unreviewed_files if > len(self.files_to_owners[file_name]) == 1] > mandatory_owners = set(files_to_owners([file_name][0] for file_name in > single_owner_files]) > for owner in mandatory_owners: > self.select_owner(owner, False) > > I think your code has the problem that if multiple files are owned by the same > single owner, you will try to select the owner multiple times. Which is probably > harmless, but perhaps not as clear as the above. > > Line 3 in my suggestion is probably also the same result what > 'self.owners.reviewers_for(single_owner_files)' would return, so that might also > be a clearer change. There won't be such a problem because once self.select_owner is called, all the files that person owns will be removed from the unreviewed_files list. During the iteration, the single_owner_files will change. The code uses generator so we don't have to create a long list every time because we always only need the first element of the list, then recreate it. If there is a way to actually search for an element with the criterion, that will be ideal.
lgtm (finally! :) ). https://codereview.chromium.org/12712002/diff/126001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode162 owners_finder.py:162: self.files_to_owners[file_name].add(owner) On 2013/09/19 23:21:43, Bei Zhang wrote: > Likewise. Only interesting files are included. > Got it, thanks. https://codereview.chromium.org/12712002/diff/126001/owners_finder.py#newcode227 owners_finder.py:227: break On 2013/09/19 23:21:43, Bei Zhang wrote: > On 2013/09/16 23:56:11, Dirk Pranke wrote: > > I think you can re-work lines 219-227 to: > > > > single_owner_files = [file_name for file_name in self.unreviewed_files if > > len(self.files_to_owners[file_name]) == 1] > > mandatory_owners = set(files_to_owners([file_name][0] for file_name in > > single_owner_files]) > > for owner in mandatory_owners: > > self.select_owner(owner, False) > > > > I think your code has the problem that if multiple files are owned by the same > > single owner, you will try to select the owner multiple times. Which is > probably > > harmless, but perhaps not as clear as the above. > > > > Line 3 in my suggestion is probably also the same result what > > 'self.owners.reviewers_for(single_owner_files)' would return, so that might > also > > be a clearer change. > > There won't be such a problem because once self.select_owner is called, all the > files that person owns will be removed from the unreviewed_files list. > > During the iteration, the single_owner_files will change. The code uses > generator so we don't have to create a long list every time because we always > only need the first element of the list, then recreate it. > > If there is a way to actually search for an element with the criterion, that > will be ideal. I see. I suppose it could be the case that if you deselect someone, that could result in other files now only having one owner. At any rate, I don't think the original algorithm was necessarily broken, just perhaps less clear than it could be. But, given what you said above, I'm not sure if my suggestions are really much better, so we can leave this as is.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ikarienator@chromium.org/12712002/138001
https://codereview.chromium.org/12712002/diff/138001/owners_finder.py File owners_finder.py (right): https://codereview.chromium.org/12712002/diff/138001/owners_finder.py#newcode18 owners_finder.py:18: COLOR_LINK = '\033[4m' Please use colorama.
Message was sent while issue was closed.
Change committed as 224264
Message was sent while issue was closed.
On 2013/09/20 02:11:50, I haz the power (commit-bot) wrote: > Change committed as 224264 Please do a follow up CL to fix my comment.
Message was sent while issue was closed.
On 2013/09/20 02:12:46, M-A Ruel wrote: > On 2013/09/20 02:11:50, I haz the power (commit-bot) wrote: > > Change committed as 224264 > > Please do a follow up CL to fix my comment. Sorry, M-A, I wasn't ignoring your feedback, I was already expecting him to address this in a follow-up CL as per comment #34. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
