Bug 72690 - ChangeLogEntry should be able to parse entries with multiple authors
Summary: ChangeLogEntry should be able to parse entries with multiple authors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 72243
  Show dependency treegraph
 
Reported: 2011-11-17 22:10 PST by Ryosuke Niwa
Modified: 2011-11-21 15:01 PST (History)
4 users (show)

See Also:


Attachments
fixes the bug (9.13 KB, patch)
2011-11-17 22:24 PST, Ryosuke Niwa
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-11-17 22:10:09 PST
It turns out that people list multiple author names in change log entries :(
Comment 1 Ryosuke Niwa 2011-11-17 22:24:45 PST
Created attachment 115741 [details]
fixes the bug
Comment 2 Ryosuke Niwa 2011-11-18 12:05:56 PST
Ping reviewers.
Comment 3 Ryosuke Niwa 2011-11-21 12:29:16 PST
Ping reviewers again
Comment 4 Eric Seidel (no email) 2011-11-21 13:09:37 PST
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 5 Ryosuke Niwa 2011-11-21 14:56:32 PST
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.
Comment 6 Ryosuke Niwa 2011-11-21 14:59:18 PST
Thanks for the review, will land now.
Comment 7 Ryosuke Niwa 2011-11-21 15:01:24 PST
Committed r100961: <http://trac.webkit.org/changeset/100961>