[webkit-reviews] review granted: [Bug 72351] Implement edit-distance based reviewer recognition algorithm : [Attachment 115270] Brought back forgotten editdistance.py

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Nov 15 16:31:07 PST 2011


Eric Seidel <eric at webkit.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 72351: Implement edit-distance based reviewer recognition algorithm
https://bugs.webkit.org/show_bug.cgi?id=72351

Attachment 115270: Brought back forgotten editdistance.py
https://bugs.webkit.org/attachment.cgi?id=115270&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115270&action=review


Looks great, as long as the reviewer() comment below isn't a bug.

> Tools/Scripts/webkitpy/common/editdistance.py:36
> +def edit_distance(str1, str2):
> +    unsignedShort = 'h'
> +    distances = [array(unsignedShort, (0,) * (len(str2) + 1)) for i in
range(0, len(str1) + 1)]
> +    for i in range(1, len(str1) + 1):
> +	   distances[i][0] = i

You might want to add a comment here to explain what you're doing.  In my 3rd
reading it's still not immediately clear what this does.  A link to a wiki page
describing the algorithm might also be helpful.

> Tools/Scripts/webkitpy/common/checkout/changelog.py:150
> +    def _fuzz_match_reviewers(self, reviewers_text_list):
> +	   if not reviewers_text_list:

Much nicer!

> Tools/Scripts/webkitpy/common/checkout/changelog.py:186
> -	   return self._reviewer  # Might be None, might also not be a
Reviewer!
> +	   return self._reviewers[0] if len(self._reviewers) > 0 else None

So won't this sometimes return an array?  If your flattening fails because
there are some lists of reviewers with more than one reviewer?

> Tools/Scripts/webkitpy/common/checkout/changelog.py:199
> +	   if re.search("unreviewed", self._contents, re.IGNORECASE):
> +	       return True
> +	   return False

Isn't this just bool(re.search...?

> Tools/Scripts/webkitpy/common/checkout/changelog_unittest.py:430
> +    def test_fuzzy_reviewer_match(self):
> +	   self._assert_fuzzy_reviewer_match('Reviewed by Dimitri Glazkov,
build fix', ['Dimitri Glazkov', 'build fix'], ['Dimitri Glazkov'])
> +	   self._assert_fuzzy_reviewer_match('Reviewed by BUILD FIX', ['BUILD
FIX'], [])
> +	   self._assert_fuzzy_reviewer_match('Reviewed by Mac build fix', ['Mac
build fix'], [])

Thank you. :)

> Tools/Scripts/webkitpy/common/config/committers.py:524
> +	       #FIXME: This should do case-insensitive comparison or assert
that all IRC nicknames are in lowercase

I think there is normaly a space between # and FIXME

> Tools/Scripts/webkitpy/common/config/committers.py:539
> +    def contributors_by_fuzzy_match(self, string):

MIND.  BLOWN.  sheriff-bot whois is drooling. :)


More information about the webkit-reviews mailing list