The suggest-nominations command does not count reviewed patches for contributors after they become a contributor. In particular, the current code only counts those patches which contain a "Patch by" line, which doesn't hold for contributors after they become committers. As a result of this, the patch count for committers are undercounted for the purpose of nominating them as reviewer. In addition, the command nominates reviewers that have not yet been confirmed as committers. The proposed patch resolves these problems by: (1) counting all qualified reviewed patches for nominations; (2) nominating reviewers only if they are existing committers; In addition, the proposed patch adds a number of useful options to obtain more information about analyzed commits: --show-skipped-commits show skipped commits except those that are explicitly marked as unreviewed --show-skipped-unreviewed-commits show skipped commits including those explicitly marked as unreviewed --show-unreviewed-commits show unmarked, unreviewed, non-gardening commits by committer/reviewer, including counts; when using this option, no nominations are output --show-unreviewed-commits-for used instead of --show-unreviewed-commits to show results for specific contributors --show-unreviewed-commits-revisions used with --show-unreviewed-commits or --show-unreviewed-commits-for in order to add list of revisions --record-marked-unreviewed-commits used with --show-unreviewed-commits or --show-unreviewed-commits-for in order to count marked, unreviewed, non-gardening commits which are normally not counted; a commit is marked as unreviewed by including 'Unreviewed' in the change log; --record-gardening-unreviewed-commits used with --record-marked-unreviewed-commits in order to additionally counted marked, unreviewed commits that include "Gardening" or "gardening"; Following is an example of using --show-unreviewed-commits (with post facto sorting of results): webkit-patch suggest-nominations --show-unreviewed-commits UNREVIEWED COMMITS: [R] (277) Ryosuke Niwa UNREVIEWED COMMITS: [R] (100) Simon Fraser UNREVIEWED COMMITS: [R] ( 95) Kent Tamura UNREVIEWED COMMITS: [R] ( 64) Dimitri Glazkov UNREVIEWED COMMITS: [R] ( 63) Ojan Vafai UNREVIEWED COMMITS: [R] ( 58) Jessie Berlin UNREVIEWED COMMITS: [R] ( 53) Geoffrey Garen UNREVIEWED COMMITS: [R] ( 47) Anders Carlsson UNREVIEWED COMMITS: [R] ( 45) Pavel Feldman UNREVIEWED COMMITS: [R] ( 44) David Kilzer UNREVIEWED COMMITS: [R] ( 43) Alexey Proskuryakov UNREVIEWED COMMITS: [R] ( 40) Sam Weinig UNREVIEWED COMMITS: [R] ( 36) Adam Barth UNREVIEWED COMMITS: [R] ( 35) James Robinson UNREVIEWED COMMITS: [R] ( 31) Oliver Hunt UNREVIEWED COMMITS: [R] ( 25) Dan Bernstein UNREVIEWED COMMITS: [R] ( 23) Csaba Osztrogonác UNREVIEWED COMMITS: [R] ( 21) Martin Robinson UNREVIEWED COMMITS: [R] ( 20) Mark Rowe UNREVIEWED COMMITS: [R] ( 19) Beth Dakin UNREVIEWED COMMITS: [R] ( 16) Dean Jackson UNREVIEWED COMMITS: [R] ( 14) Andreas Kling UNREVIEWED COMMITS: [R] ( 12) Andy Estes UNREVIEWED COMMITS: [R] ( 10) Benjamin Poulain UNREVIEWED COMMITS: [R] ( 9) Levi Weintraub UNREVIEWED COMMITS: [R] ( 9) Daniel Bates UNREVIEWED COMMITS: [R] ( 9) Brady Eidson UNREVIEWED COMMITS: [R] ( 8) Filip Pizlo UNREVIEWED COMMITS: [R] ( 7) Simon Hausmann UNREVIEWED COMMITS: [R] ( 7) Julien Chaffraix UNREVIEWED COMMITS: [R] ( 6) Nate Chapin UNREVIEWED COMMITS: [R] ( 6) Gavin Barraclough UNREVIEWED COMMITS: [R] ( 6) Brent Fulgham UNREVIEWED COMMITS: [R] ( 5) Timothy Hatcher UNREVIEWED COMMITS: [R] ( 4) Michael Saboff UNREVIEWED COMMITS: [R] ( 4) Mark Hahnenberg UNREVIEWED COMMITS: [R] ( 4) Jer Noble UNREVIEWED COMMITS: [R] ( 4) Dirk Pranke UNREVIEWED COMMITS: [R] ( 4) Darin Adler UNREVIEWED COMMITS: [R] ( 3) Chris Fleizach UNREVIEWED COMMITS: [R] ( 2) Yury Semikhatsky UNREVIEWED COMMITS: [R] ( 2) Xan Lopez UNREVIEWED COMMITS: [R] ( 2) Tim Horton UNREVIEWED COMMITS: [R] ( 2) Kentaro Hara UNREVIEWED COMMITS: [R] ( 2) Dmitry Titov UNREVIEWED COMMITS: [R] ( 2) David Hyatt UNREVIEWED COMMITS: [R] ( 2) Antti Koivisto UNREVIEWED COMMITS: [R] ( 1) Zoltan Herczeg UNREVIEWED COMMITS: [R] ( 1) Philip Rogers UNREVIEWED COMMITS: [R] ( 1) Gustavo Noronha Silva UNREVIEWED COMMITS: [R] ( 1) Eric Seidel UNREVIEWED COMMITS: [R] ( 1) Brian Weinstein UNREVIEWED COMMITS: [R] ( 1) Antonio Gomes UNREVIEWED COMMITS: [C] (102) Raphael Kubo da Costa UNREVIEWED COMMITS: [C] ( 47) Patrick Gansterer UNREVIEWED COMMITS: [C] ( 44) Roger Fong UNREVIEWED COMMITS: [C] ( 35) Lucas Forschler UNREVIEWED COMMITS: [C] ( 22) Jon Lee UNREVIEWED COMMITS: [C] ( 15) Mark Lam UNREVIEWED COMMITS: [C] ( 10) Hin-Chung Lam UNREVIEWED COMMITS: [C] ( 6) Yoshifumi Inoue UNREVIEWED COMMITS: [C] ( 6) Fady Samuel UNREVIEWED COMMITS: [C] ( 6) Adam Kallai UNREVIEWED COMMITS: [C] ( 5) Žan Doberšek UNREVIEWED COMMITS: [C] ( 5) Pavel Podivilov UNREVIEWED COMMITS: [C] ( 4) Balazs Kelemen UNREVIEWED COMMITS: [C] ( 3) Peter Kasting UNREVIEWED COMMITS: [C] ( 3) Ilya Tikhonovsky UNREVIEWED COMMITS: [C] ( 3) Chris Evans UNREVIEWED COMMITS: [C] ( 2) Takashi Sakamoto UNREVIEWED COMMITS: [C] ( 2) Pierre Rossi UNREVIEWED COMMITS: [C] ( 2) Kenichi Ishibashi UNREVIEWED COMMITS: [C] ( 2) Karen Grunberg UNREVIEWED COMMITS: [C] ( 2) Antoine Quint UNREVIEWED COMMITS: [C] ( 2) Andrey Adaykin UNREVIEWED COMMITS: [C] ( 2) Adam Klein UNREVIEWED COMMITS: [C] ( 1) Xiaohai Wei UNREVIEWED COMMITS: [C] ( 1) Xianzhu Wang UNREVIEWED COMMITS: [C] ( 1) W. James MacLean UNREVIEWED COMMITS: [C] ( 1) Vincent Scheib UNREVIEWED COMMITS: [C] ( 1) Terry Anderson UNREVIEWED COMMITS: [C] ( 1) Silvia Pfeiffer UNREVIEWED COMMITS: [C] ( 1) Sadrul Habib Chowdhury UNREVIEWED COMMITS: [C] ( 1) Raymond Toy UNREVIEWED COMMITS: [C] ( 1) Rafael Weinstein UNREVIEWED COMMITS: [C] ( 1) Mikhail Pozdnyakov UNREVIEWED COMMITS: [C] ( 1) Mike Lawther UNREVIEWED COMMITS: [C] ( 1) Michael Nordman UNREVIEWED COMMITS: [C] ( 1) Mark Pilgrim UNREVIEWED COMMITS: [C] ( 1) Luciano Wolf UNREVIEWED COMMITS: [C] ( 1) Leo Yang UNREVIEWED COMMITS: [C] ( 1) Lauro Neto UNREVIEWED COMMITS: [C] ( 1) Jesus Sanchez-Palencia UNREVIEWED COMMITS: [C] ( 1) Jeffrey Pfau UNREVIEWED COMMITS: [C] ( 1) Hayato Ito UNREVIEWED COMMITS: [C] ( 1) Dominic Mazzoni UNREVIEWED COMMITS: [C] ( 1) Dmitry Gorbik UNREVIEWED COMMITS: [C] ( 1) Dave Barton UNREVIEWED COMMITS: [C] ( 1) Dana Jansens UNREVIEWED COMMITS: [C] ( 1) Andras Becsi UNREVIEWED COMMITS: Total 1660
Created attachment 199870 [details] Patch
(In reply to comment #1) > Created an attachment (id=199870) [details] > Patch I should also mention that this patch divides the prior functionality of SuggestNominations into AbstractSuggestCommand and SuggestNominations. This was done in order to prepare for the addition of a SuggestEmeriti class in order to add a suggest-emeriti command in a separate, upcoming patch, where both of these commands reuse the functionality in the base AbstractSuggestCommand.
Created attachment 199918 [details] Patch
(In reply to comment #3) > Created an attachment (id=199918) [details] > Patch Defer addition of support for additional commit analysis output, which is better targeted to a new command 'analyze-commits'. Once the present patch is landed (mutatis mutants) I will open a new bug with a separate patch to add such a command. This allows the current patch to be smaller and focus on abstracting base functionality into the new AbstractCommitLogCommand class and make the bug fixes noted in this bug.
(In reply to comment #0) > The suggest-nominations command does not count reviewed patches for contributors after they become a contributor. s/after they become a contributor/after they become a committer/
Created attachment 199960 [details] Remove duplication changelog entry.
Comment on attachment 199960 [details] Remove duplication changelog entry. Too many changes at once. Please don't include the changes to add AbstractCommitLogCommand in this patch.
Created attachment 200036 [details] Patch
(In reply to comment #8) > Created an attachment (id=200036) [details] > Patch Removed AbstractCommitLogCommand and simplified changes as requested by rniwa.
Comment on attachment 200036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200036&action=review > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:48 > + _match_leading_indent = re.compile(r"^[ ]{4}", re.MULTILINE) > + _match_reviewed_by = re.compile(r'^\s*((\w+\s+)+and\s+)?(Review|Rubber(\s*|-)stamp)(s|ed)?\s+([a-z]+\s+)*?by\s+(?P<reviewers>.*?)[\.,]?\s*$', re.MULTILINE) > + _match_patch_by = re.compile(r'^Patch by (?P<name>.+?)\s+<(?P<email>[^<>]+)> on (?P<date>\d{4}-\d{2}-\d{2})$', re.MULTILINE) > + _match_committer = re.compile(r'^Author: (?P<email>\S+)\s+<[^>]+>$', re.MULTILINE) > + _match_date = re.compile(r'^Date: \S{3} (?P<month>\S{3}) (?P<day>\d{1,2}) (?P<time>\d{2}:\d{2}:\d{2}) (?P<year>\d{4}) [\+\-]\d{4}$', re.MULTILINE) > + _match_revision = re.compile(r'^git-svn-id: http://svn.webkit.org/repository/webkit/trunk@(?P<svnid>\d+) (?P<gitid>[0-9a-f\-]{36})$', re.MULTILINE) Our naming convection is to postfix these names with _regex instead of prefixing them with match since they're not match objects. Looks like there's a lot of code duplication with ChangeLogEntry at http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/checkout/changelog.py#L60 > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:96 > + def _recent_commits(self): Looks like much of code in this function can be substituted by some combinations of method calls in ChangeLogEntry if we generalize the class well enough. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:175 > + for commit in self._recent_commits(): Looks like it's completely unnecessary for _recent_commits to yield. This function can iterate over self._recent_commit_messages() and call a function that analyzes the commit and returns None when it needs to be skipped.
Created attachment 200055 [details] Patch
(In reply to comment #10) > (From update of attachment 200036 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=200036&action=review > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:48 > > + _match_leading_indent = re.compile(r"^[ ]{4}", re.MULTILINE) > > + _match_reviewed_by = re.compile(r'^\s*((\w+\s+)+and\s+)?(Review|Rubber(\s*|-)stamp)(s|ed)?\s+([a-z]+\s+)*?by\s+(?P<reviewers>.*?)[\.,]?\s*$', re.MULTILINE) > > + _match_patch_by = re.compile(r'^Patch by (?P<name>.+?)\s+<(?P<email>[^<>]+)> on (?P<date>\d{4}-\d{2}-\d{2})$', re.MULTILINE) > > + _match_committer = re.compile(r'^Author: (?P<email>\S+)\s+<[^>]+>$', re.MULTILINE) > > + _match_date = re.compile(r'^Date: \S{3} (?P<month>\S{3}) (?P<day>\d{1,2}) (?P<time>\d{2}:\d{2}:\d{2}) (?P<year>\d{4}) [\+\-]\d{4}$', re.MULTILINE) > > + _match_revision = re.compile(r'^git-svn-id: http://svn.webkit.org/repository/webkit/trunk@(?P<svnid>\d+) (?P<gitid>[0-9a-f\-]{36})$', re.MULTILINE) > > Our naming convection is to postfix these names with _regex instead of prefixing them with match since they're not match objects. Change to _regexp suffix, such as used in changelogentry.py. > Looks like there's a lot of code duplication with ChangeLogEntry at http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/checkout/changelog.py#L60 Turns out to be very little overlap. In particular, only reuse is ChangeLogEntry.reviewed_by_regexp, which I've reused. The reason there is little overlap is because: (1) commit log doesn't contain change log date line, but uses git log's 'Date: ' line; (2) determination of author uses different algorithm, i.e., uses 'Patch by' line (from change log body) first then uses 'Author: ' git log line second > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:96 > > + def _recent_commits(self): > > Looks like much of code in this function can be substituted by some combinations of method calls in ChangeLogEntry if we generalize the class well enough. Not much. See above. Change log and commit log analysis use different algorithms and information to determine committer, author, and date. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:175 > > + for commit in self._recent_commits(): > > Looks like it's completely unnecessary for _recent_commits to yield. This function can iterate over self._recent_commit_messages() > and call a function that analyzes the commit and returns None when it needs to be skipped. I swapped this out with iteration over messages invoking an analyzer callback that filters entries and extracts state as needed.
Comment on attachment 200055 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200055&action=review > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:99 > + # Determine committer if possible, otherwise skip commit. We should share code with ChangeLogEntry as I mentioned earlier. r-. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:160 > + on_analysis(analysis, committer, commit_date, revision, author_email, author_name, contributor, reviewers) Why do we need a callback function?
(In reply to comment #13) > (From update of attachment 200055 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=200055&action=review > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:99 > > + # Determine committer if possible, otherwise skip commit. > > We should share code with ChangeLogEntry as I mentioned earlier. r-. I explained in comment #12 below why I have shared all that can be reasonably shared. Change log and commit log entries use different data sets, and different semantics for what constitutes author. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:160 > > + on_analysis(analysis, committer, commit_date, revision, author_email, author_name, contributor, reviewers) > > Why do we need a callback function? How would your propose to write this logic then? It worked fine as a generator, and you asked it to be change to "call a function that analyzes the commit".
Comment on attachment 200055 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200055&action=review > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:166 > + def _analyze_commits(self, on_analysis, analysis, analyze=None): > + if not analyze: > + analyze = self._analyze_commit > + for commit_message in self._recent_commit_messages(): > + analyze(commit_message, on_analysis, analysis) This should be merged into the caller of _analyze_commits. This function doesn't do anything useful and only adds undesirable indirections. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:213 > + self._analyze_commits(_on_analysis, analysis) This is the sole caller of _analyze_commits. We shouldn't need to make it take two callbacks. We don't any more premature/over generalization in webkitpy. It's bad as is.
(In reply to comment #15) > (From update of attachment 200055 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=200055&action=review > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:166 > > + def _analyze_commits(self, on_analysis, analysis, analyze=None): > > + if not analyze: > > + analyze = self._analyze_commit > > + for commit_message in self._recent_commit_messages(): > > + analyze(commit_message, on_analysis, analysis) > > This should be merged into the caller of _analyze_commits. This function doesn't do anything useful and only adds undesirable indirections. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:213 > > + self._analyze_commits(_on_analysis, analysis) > > This is the sole caller of _analyze_commits. We shouldn't need to make it take two callbacks. > We don't any more premature/over generalization in webkitpy. It's bad as is. I disagree. There is a perfectly good reason to do this. At your request, I have already (temporarily) backed out the addition of an abstract commit log command base class, but there is good cause to begin to generalize the analysis process to support [1][2][3]: [1] https://bugs.webkit.org/show_bug.cgi?id=115387 [2] https://bugs.webkit.org/show_bug.cgi?id=115388 [3] https://bugs.webkit.org/show_bug.cgi?id=115391
Created attachment 200065 [details] Patch
Comment on attachment 200065 [details] Patch I agree with Ryosuke here, I don't get _analyze_commits. Especially the last, never used, argument. If you need the function for future change, just add it with those changes. I would also modify contributors.json separately from this patch. > > This is the sole caller of _analyze_commits. We shouldn't need to make it take two callbacks. > > We don't any more premature/over generalization in webkitpy. It's bad as is. > > I disagree. There is a perfectly good reason to do this. What is the good reason? This is the kind of information best detailed in the ChangeLog.
Created attachment 200068 [details] Patch
(In reply to comment #18) > (From update of attachment 200065 [details]) > I agree with Ryosuke here, I don't get _analyze_commits. Especially the last, never used, argument. > > If you need the function for future change, just add it with those changes. > > I would also modify contributors.json separately from this patch. Done. > > > > This is the sole caller of _analyze_commits. We shouldn't need to make it take two callbacks. > > > We don't any more premature/over generalization in webkitpy. It's bad as is. > > > > I disagree. There is a perfectly good reason to do this. > > What is the good reason? > This is the kind of information best detailed in the ChangeLog. Removed _analyze_commits, recoding inline.
Comment on attachment 200068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200068&action=review Comments bellow. Personally I think it is okay to parse commit messages differently than ChangeLog. But realistically Ryosuke is the maintainer of webkitpy so better get his input too. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:91 > + def _commit_date_from_match(self, match): > + year = match.group('year') > + month_abbr = match.group('month') > + month = 0 > + for i in range(len(calendar.month_abbr)): > + if calendar.month_abbr[i] == month_abbr: > + month = i + 1 > + break > + day = match.group('day') > + return '%04d-%02d-%02d' % (int(year), month, int(day)) Instead, I could just have git give the nicely formatted date. Look for --pretty in the documentation. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:97 > + def _analyze_commit(self, commit_message, on_analysis, analysis): The code of this function is made extremely fragile by all the if not match return Maybe it should instead raise an exception when it fails parsing? The caller code can then catch the exceptions and notify the user of how many commits were skipped. It would not prevent the code from breaking, but at least the user of the command would know it is broken. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:98 > + Empty line. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:107 > + # Determine committer if possible, otherwise skip commit. > + committer_match = self._committer_regexp.search(commit_message) > + if not committer_match: > + return > + > + # Determine committer email if possible, otherwise skip commit. > + committer_email = committer_match.group('email') > + if not committer_email: > + return This looks like the common pattern. Maybe make a function out of it so that each of those 4 lines group become just one line. The comments are not adding information here. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:109 > + # Determine if committer is a known contributor, i.e., registered in committers.py, otherwise skip commit. This sounds like a valid case for failing, no? > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:154 > + # Determine reviewers if possible, otherwise optionally skip commit. optionally? > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:209 > + for commit_message in self._recent_commit_messages(): > + self._analyze_commit(commit_message, _on_analysis, analysis) Why does this with the callback _on_analysis? What about renaming _analyze_commit to parse_commit. It returns a simple object that you pass to a _add_to_contribution_histogram or something?
Created attachment 200079 [details] Patch
(In reply to comment #21) > (From update of attachment 200068 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=200068&action=review > > Comments bellow. > > Personally I think it is okay to parse commit messages differently than ChangeLog. But realistically Ryosuke is the maintainer of webkitpy so better get his input too. I already discussed with Ryosuke. If he has any specific suggested changes, I'd be happy to apply them. I'm not really changing anything in this patch with respect to how the commit log data is parsed. It was already different than the change log parser before this patch, and nothing is being changed here in that respect. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:91 > > + def _commit_date_from_match(self, match): > > + year = match.group('year') > > + month_abbr = match.group('month') > > + month = 0 > > + for i in range(len(calendar.month_abbr)): > > + if calendar.month_abbr[i] == month_abbr: > > + month = i + 1 > > + break > > + day = match.group('day') > > + return '%04d-%02d-%02d' % (int(year), month, int(day)) > > Instead, I could just have git give the nicely formatted date. > Look for --pretty in the documentation. Fixed. Use --date=iso option on git log, replace regex. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:97 > > + def _analyze_commit(self, commit_message, on_analysis, analysis): > > The code of this function is made extremely fragile by all the > if not match > return Well, no more fragile than the existing code, which used if not match continue. In any case, I've fixed by adding exceptions as you suggest below. > > Maybe it should instead raise an exception when it fails parsing? > The caller code can then catch the exceptions and notify the user of how many commits were skipped. It would not prevent the code from breaking, but at least the user of the command would know it is broken. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:98 > > + > > Empty line. Fixed. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:107 > > + # Determine committer if possible, otherwise skip commit. > > + committer_match = self._committer_regexp.search(commit_message) > > + if not committer_match: > > + return > > + > > + # Determine committer email if possible, otherwise skip commit. > > + committer_email = committer_match.group('email') > > + if not committer_email: > > + return > > This looks like the common pattern. Maybe make a function out of it so that each of those 4 lines group become just one line. Doing this makes the code harder to read by my estimation and doesn't really buy much. I'd prefer not to do this unless you insist. > > The comments are not adding information here. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:109 > > + # Determine if committer is a known contributor, i.e., registered in committers.py, otherwise skip commit. > > This sounds like a valid case for failing, no? Reordered the code a bit, but contributor == None isn't an error condition, it merely means the author (as previously determined) does not yet have an entry in the contributors list. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:154 > > + # Determine reviewers if possible, otherwise optionally skip commit. > > optionally? Now throws CommitLogMissingReviewer. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:209 > > + for commit_message in self._recent_commit_messages(): > > + self._analyze_commit(commit_message, _on_analysis, analysis) > > Why does this with the callback _on_analysis? > > What about renaming _analyze_commit to parse_commit. It returns a simple object that you pass to a _add_to_contribution_histogram or something? You guys are killing me. That's what I did to start with, via a generator and yield, but Ryosuke asked to change to a callback. Can we keep it as is? I'm going to be tweaking this very soon anyway in order to address [1]. [1] https://bugs.webkit.org/show_bug.cgi?id=115391
Comment on attachment 200079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200079&action=review I would really prefer the callback approach to be cleaned but I am too tired to fight. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:40 > + Blank line. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:46 > + Blank line. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:53 > - help_text = "Suggest contributors for committer/reviewer nominations" > + help_text = "Suggest nominations for committer/reviewer role" I think the previous text is better. The patch count is only a lower limit. You can have thousands of patches and not be nominated. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:126 > + if not author_match: > + raise CommitLogError How could this happen? We could not get here if committer_match is None. > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:132 > + if not author_email: > + raise CommitLogError ditto
Created attachment 200196 [details] Patch for landing
(In reply to comment #24) > (From update of attachment 200079 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=200079&action=review > > I would really prefer the callback approach to be cleaned but I am too tired to fight. I went ahead and recoded this as you suggested. See _parse_commit_message and _count_commit. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:40 > > + > > Blank line. Fixed. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:46 > > + > > Blank line. Fixed. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:53 > > - help_text = "Suggest contributors for committer/reviewer nominations" > > + help_text = "Suggest nominations for committer/reviewer role" > > I think the previous text is better. Fixed. > > The patch count is only a lower limit. You can have thousands of patches and not be nominated. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:126 > > + if not author_match: > > + raise CommitLogError > > How could this happen? > We could not get here if committer_match is None. Fixed. > > > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:132 > > + if not author_email: > > + raise CommitLogError > > ditto Fixed.
Comment on attachment 200196 [details] Patch for landing Clearing flags on attachment: 200196 Committed r149419: <http://trac.webkit.org/changeset/149419>
All reviewed patches have been landed. Closing bug.
Comment on attachment 200196 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=200196&action=review > Tools/Scripts/webkitpy/tool/commands/suggestnominations.py:81 > + messages = re.split(r"^commit \w{40}$", git_log, flags=re.MULTILINE)[1:] # Ignore the first message which will be empty. Can't use this. Only Python 2.7 supports flags argument, and we still need to support Python 2.6 on Windows. Fixed it in http://trac.webkit.org/changeset/150461.