It turns out that people list multiple author names in change log entries :(
Created attachment 115741 [details] fixes the bug
Ping reviewers.
Ping reviewers again
Comment on attachment 115741 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=115741&action=review > Tools/Scripts/webkitpy/common/checkout/changelog.py:-140 > - # FIXME: Canonicalize reviewer names; e.g. Andy "First Time Reviewer" Estes > - # FIXME: Ignore NOBODY (\w+) and "a spell checker" > - reviewer_list = re.split(r'\s*(?:(?:,(?:\s+and\s+|&)?)|(?:and\s+|&)|(?:[/+]))\s*', reviewer_text) Did these get fixed? > Tools/Scripts/webkitpy/common/checkout/changelog.py:171 > + self._authors = [] > + if self._author_text: > + authors = ChangeLogEntry._split_contributor_names(self._author_text) > + assert(authors and len(authors) >= 1) > + for author in authors: > + author_match = re.match(r'(?P<name>.+?)\s+<(?P<email>[^>]+)>', author) > + self._authors.append({'name': author_match.group("name"), 'email': author_match.group("email")}) Any time I find myself initializing an empty array and adding to it, I wonder if I should use a list comprehension. Furthermore, it seems like this should just be a helper function: self._authors = self._parse_author_text(self._author_text) > Tools/Scripts/webkitpy/common/checkout/changelog.py:183 > - return self._author_name > + return self._authors[0]['name'] Do people really put more than one author in the date line? > Tools/Scripts/webkitpy/common/config/committers.py:612 > + return self._reviewer_only(self.account_by_email(email)) if email else None These ones, where you aren't calling foo.lower() aren't needed. YOu can just leave them as they were, no? .get() will return None if passed None in this case, since None is not a key in the dictionary (or shouldn't be!)
Comment on attachment 115741 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=115741&action=review >> Tools/Scripts/webkitpy/common/checkout/changelog.py:-140 >> - reviewer_list = re.split(r'\s*(?:(?:,(?:\s+and\s+|&)?)|(?:and\s+|&)|(?:[/+]))\s*', reviewer_text) > > Did these get fixed? Yup. Done in http://trac.webkit.org/changeset/100233. >> Tools/Scripts/webkitpy/common/checkout/changelog.py:171 >> + self._authors.append({'name': author_match.group("name"), 'email': author_match.group("email")}) > > Any time I find myself initializing an empty array and adding to it, I wonder if I should use a list comprehension. > > Furthermore, it seems like this should just be a helper function: > self._authors = self._parse_author_text(self._author_text) Hm... okay will try. >> Tools/Scripts/webkitpy/common/checkout/changelog.py:183 >> + return self._authors[0]['name'] > > Do people really put more than one author in the date line? Yes. e.g. Look for "Zan Dobersek <zandobersek@gmail.com> and Philippe Normand <pnormand@igalia.com>" >> Tools/Scripts/webkitpy/common/config/committers.py:612 >> + return self._reviewer_only(self.account_by_email(email)) if email else None > > These ones, where you aren't calling foo.lower() aren't needed. YOu can just leave them as they were, no? .get() will return None if passed None in this case, since None is not a key in the dictionary (or shouldn't be!) Good point. Will revert.
Thanks for the review, will land now.
Committed r100961: <http://trac.webkit.org/changeset/100961>