[webkit-reviews] review denied: [Bug 115285] [webkitpy] suggest-nominations doesn't count all qualified patches : [Attachment 200068] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 29 20:03:57 PDT 2013


Benjamin Poulain <benjamin at webkit.org> has denied Glenn Adams
<glenn at skynav.com>'s request for review:
Bug 115285: [webkitpy] suggest-nominations doesn't count all qualified patches
https://bugs.webkit.org/show_bug.cgi?id=115285

Attachment 200068: Patch
https://bugs.webkit.org/attachment.cgi?id=200068&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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?


More information about the webkit-reviews mailing list