Bug 106184 – sheriffbot can't tell me who "kov" is

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

[Eric Seidel (no email)]

sheriffbot can't tell me who "kov" is

I don't know of anyway to get an exact match. :(

2:00 PM <eseidel> sheriffbot: whois kov
2:00 PM <sheriffbot> eseidel: More than 5 contributors match 'kov', could you be more specific?
2:01 PM <eseidel> sheriffbot: whois kov@
2:01 PM <sheriffbot> eseidel: I'm not sure who you mean?  MPozdnyakov, dglazkov, or kov could be 'kov@'.

[Alan Cutter]

Adding the ability to use double-quotes for exact searches.

23:13:42 @alancutter | sheriffbot: whois kov
23:13:44  sheriffbot | alancutter: More than 5 contributors match 'kov', could you be more specific? Try using double-quotes for an exact search.
23:13:46 @alancutter | sheriffbot: whois kov@
23:13:54  sheriffbot | alancutter: I'm not sure who you mean?  MPozdnyakov, dglazkov, or kov could be 'kov@'. Try using double-quotes for an exact search.
23:14:00 @alancutter | sheriffbot: whois "kov"
23:14:04  sheriffbot | alancutter: kov is kov ([e-mail], [e-mail], [e-mail], [e-mail]). Why do you ask?

[Alan Cutter]

Created attachment 181467 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 181467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181467&action=review

Looks reasonable.   I'm not sure I would have added the quotes feature.  I might instead have just had SB prefer exact matches, and display them, while still teling them there were 5 other matches.  Or more generally SB could just display the best match always, and tell you that it has 5 more...

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:275
> +                else:
> +                    return "%s: %s is %s. Why do you ask?" % (nick, search_string, self._nick_or_full_record(contributor))

No else needed after return.  The "Why do you ask" is a bit silly, but I guess that's part of SB's "personality" :)

> Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py:42
> +        self.assertEqual('tom: Usage: whois SEARCH_STRING\t// Surround with double-quotes for exact match eg. "username"',

Is the // supposed to feel like a c++ comment?

[Eric Seidel (no email)]

An example of what I might have done:

2:00 PM <eseidel> sheriffbot: whois kov
2:00 PM <sherrifbot> eseidel: The best match for kov is kov ([e-mail], [e-mail], [e-mail], [e-mail]).
2:00 PM <sheriffbot> eseidel: But 4 other contributors match 'kov', could you be more specific?

[Alan Cutter]

Created attachment 181468 [details]
Patch

[Alan Cutter]

(In reply to comment #3)
> (From update of attachment 181467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181467&action=review
> 
> Looks reasonable.   I'm not sure I would have added the quotes feature.  I might instead have just had SB prefer exact matches, and display them, while still teling them there were 5 other matches.  Or more generally SB could just display the best match always, and tell you that it has 5 more...
> 
> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:275
> > +                else:
> > +                    return "%s: %s is %s. Why do you ask?" % (nick, search_string, self._nick_or_full_record(contributor))
> 
> No else needed after return.  The "Why do you ask" is a bit silly, but I guess that's part of SB's "personality" :)
> 

Removed else statement (existing code).

> > Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py:42
> > +        self.assertEqual('tom: Usage: whois SEARCH_STRING\t// Surround with double-quotes for exact match eg. "username"',
> 
> Is the // supposed to feel like a c++ comment?

I was unsure of how to add extra information to the usage response. At the moment sheriffbot can only to one-liner replies. I have a mind to add the ability to do multiline replies.

[Dirk Pranke]

Comment on attachment 181468 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181468&action=review

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:245
> +            return '%s: Usage: whois SEARCH_STRING\t// Surround with double-quotes for exact match eg. "username"' % nick

Don't use "//" for the help string, use parenthesis or something that is otherwise grammatically correct.

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:248
>          contributors = CommitterList().contributors_by_search_string(search_string)

Most of this patch feels like logic that should be pushed into CommitterList (the "model") rather than done here (the view/controller). The other irc commands are very thin wrappers around functionality present elsewhere in  webkitpy.

Also, I'm at least a bit puzzled that, for this specific example, we wouldn't just return one match for "kov" and leave it at that. Personally I would've just done a glob-style match across all of the string fields (so "kov" would match only [e-mail] and you'd have to specify "sheriffbot: whois *kov*" to get a list of other possible matches. I would also be fine with returning results ranked in some way. The "fuzzy by default but add additional code for exact matching" feels like overkill to me, but I wouldn't r- the patch just for that.

[Alan Cutter]

Created attachment 183106 [details]
Patch

[Alan Cutter]

(In reply to comment #7)
> (From update of attachment 181468 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181468&action=review
> 
> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:245
> > +            return '%s: Usage: whois SEARCH_STRING\t// Surround with double-quotes for exact match eg. "username"' % nick
> 
> Don't use "//" for the help string, use parenthesis or something that is otherwise grammatically correct.
> 
> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:248
> >          contributors = CommitterList().contributors_by_search_string(search_string)
> 
> Most of this patch feels like logic that should be pushed into CommitterList (the "model") rather than done here (the view/controller). The other irc commands are very thin wrappers around functionality present elsewhere in  webkitpy.
> 
> Also, I'm at least a bit puzzled that, for this specific example, we wouldn't just return one match for "kov" and leave it at that. Personally I would've just done a glob-style match across all of the string fields (so "kov" would match only [e-mail] and you'd have to specify "sheriffbot: whois *kov*" to get a list of other possible matches. I would also be fine with returning results ranked in some way. The "fuzzy by default but add additional code for exact matching" feels like overkill to me, but I wouldn't r- the patch just for that.


Excellent suggestion! I switched to glob style searching with the original search in string method as a fallback and moved the logic to committers.py. The complexity of the patch was significantly reduced.
Amendment attached.

[Eric Seidel (no email)]

Comment on attachment 183106 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 183106 [details]
Patch

Clearing flags on attachment: 183106

Committed r139969: <http://trac.webkit.org/changeset/139969>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.