Bug 72690 – ChangeLogEntry should be able to parse entries with multiple authors

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

It turns out that people list multiple author names in change log entries :(

[Ryosuke Niwa]

Created attachment 115741 [details]
fixes the bug

[Ryosuke Niwa]

Ping reviewers.

[Ryosuke Niwa]

Ping reviewers again

[Eric Seidel (no email)]

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!)

[Ryosuke Niwa]

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  <[e-mail]> and Philippe Normand  <[e-mail]>"

>> 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.

[Ryosuke Niwa]

Thanks for the review, will land now.

[Ryosuke Niwa]

Committed r100961: <http://trac.webkit.org/changeset/100961>