[webkit-reviews] review granted: [Bug 72690] ChangeLogEntry should be able to parse entries with multiple authors : [Attachment 115741] fixes the bug

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 72690: ChangeLogEntry should be able to parse entries with multiple authors
https://bugs.webkit.org/show_bug.cgi?id=72690

Attachment 115741: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=115741&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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!)