81101 2012-03-14 06:24:48 -0700 sheriffbot should also be addressable with a comma in addition to colon 2012-03-15 05:50:43 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 gustavo gustavo abarth ojan webkit.review.bot oldest_to_newest 578348 0 gustavo 2012-03-14 06:24:48 -0700 sheriffbot should also be addressable with a comma in addition to colon 578350 1 131836 gustavo 2012-03-14 06:28:30 -0700 Created attachment 131836 Patch 578351 2 gustavo 2012-03-14 06:30:21 -0700 The IRC client I use adds a comma after the nickname, when tab-completing, instead of a :. sheriffbot ignores those messages, would be good to allow both. 578814 3 131836 abarth 2012-03-14 15:00:49 -0700 Comment on attachment 131836 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131836&action=review > Tools/Scripts/webkitpy/common/net/irc/ircbot.py:79 > + if not irclib.irc_lower(request[:nickname_length]).startswith(irclib.irc_lower(self.connection.get_nickname())): What's the point of trimming request to nickname_length if you're going to use startswith anyway? > Tools/Scripts/webkitpy/common/net/irc/ircbot.py:84 > + if request[nickname_length] == ':': Won't this crash if len(request) == nickname_length ? > Tools/Scripts/webkitpy/common/net/irc/ircbot.py:85 > + request = request.split(':') Also, don't we need to use split(":", 1) to avoid breaking in cases like: sheriffbot: some message with a : character ? 578862 4 131941 gustavo 2012-03-14 15:40:12 -0700 Created attachment 131941 Patch 578864 5 gustavo 2012-03-14 15:41:59 -0700 (In reply to comment #3) > > Tools/Scripts/webkitpy/common/net/irc/ircbot.py:79 > > + if not irclib.irc_lower(request[:nickname_length]).startswith(irclib.irc_lower(self.connection.get_nickname())): > > What's the point of trimming request to nickname_length if you're going to use startswith anyway? The idea was to only apply irc_lower on what was needed for the test, but it's probably a worthless micro-optimization anyway. > > Tools/Scripts/webkitpy/common/net/irc/ircbot.py:85 > > + request = request.split(':') > > Also, don't we need to use split(":", 1) to avoid breaking in cases like: > > sheriffbot: some message with a : character And the original code even did it, properly =). Thanks for the review! 578873 6 131941 abarth 2012-03-14 15:53:12 -0700 Comment on attachment 131941 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131941&action=review > Tools/Scripts/webkitpy/common/net/irc/ircbot.py:86 > + # Some IRC clients, like xchat-gnome, default to using a comma > + # when addressing someone. > + try: > + vocative_separator = request[len(connection.get_nickname())] > + except IndexError: > + return This patch is fine, but you don't really need to use a try-catch block here. A simple length test should work fine. :) 579301 7 gustavo 2012-03-15 05:50:43 -0700 Committed r110837: <http://trac.webkit.org/changeset/110837> 131836 2012-03-14 06:28:30 -0700 2012-03-14 15:40:06 -0700 Patch bug-81101-20120314102828.patch text/plain 2098 gustavo U3VidmVyc2lvbiBSZXZpc2lvbjogMTEwNTYxCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggMmZlZjYzMDJlOTA1NDUzZTVlNDU0N2JiMTFiYWNjM2Ez M2U2NjM3My4uODc1NjQyMzBjNWRkNjY2OTIxNWRhODViMDJjMTU1YzU4ZjZiMjZkMCAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEz IEBACisyMDEyLTAzLTE0ICBHdXN0YXZvIE5vcm9uaGEgU2lsdmEgIDxnbnNAZ25vbWUub3JnPgor CisgICAgICAgIHNoZXJpZmZib3Qgc2hvdWxkIGFsc28gYmUgYWRkcmVzc2FibGUgd2l0aCBhIGNv bW1hIGluIGFkZGl0aW9uIHRvIGNvbG9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD04MTEwMQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9jb21tb24vbmV0L2lyYy9pcmNib3Qu cHk6CisgICAgICAgIChJUkNCb3Qub25fcHVibXNnKToKKwogMjAxMi0wMy0xMiAgT2phbiBWYWZh aSAgPG9qYW5AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIHdlYmtpdC1wYXRjaCBvcHRpbWl6ZS1i YXNlbGluZXMgc29tZXRpbWVzIGNyZWF0ZXMgYSBtYWMtZnV0dXJlIHJlc3VsdApkaWZmIC0tZ2l0 IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vbmV0L2lyYy9pcmNib3QucHkgYi9Ub29s cy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9uZXQvaXJjL2lyY2JvdC5weQppbmRleCAwNjFhNDNj ZjYyYjg2YTAyNzhjNmM1ODg4ZDY1MTUzMjcxODYzZDI0Li5lYTJkZjBiNTVhYzBlNmU1ZGQ3ZWVl MmI0NDJmZDUxOWFhZjkzOWZiIDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2Nv bW1vbi9uZXQvaXJjL2lyY2JvdC5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1v bi9uZXQvaXJjL2lyY2JvdC5weQpAQCAtNzMsOCArNzMsMjIgQEAgY2xhc3MgSVJDQm90KGlyY2Jv dC5TaW5nbGVTZXJ2ZXJJUkNCb3QsIE1lc3NhZ2VQdW1wRGVsZWdhdGUpOgogCiAgICAgZGVmIG9u X3B1Ym1zZyhzZWxmLCBjb25uZWN0aW9uLCBldmVudCk6CiAgICAgICAgIG5pY2sgPSBpcmNsaWIu bm1fdG9fbihldmVudC5zb3VyY2UoKSkKLSAgICAgICAgcmVxdWVzdCA9IGV2ZW50LmFyZ3VtZW50 cygpWzBdLnNwbGl0KCI6IiwgMSkKLSAgICAgICAgaWYgbGVuKHJlcXVlc3QpID4gMSBhbmQgaXJj bGliLmlyY19sb3dlcihyZXF1ZXN0WzBdKSA9PSBpcmNsaWIuaXJjX2xvd2VyKHNlbGYuY29ubmVj dGlvbi5nZXRfbmlja25hbWUoKSk6CisgICAgICAgIHJlcXVlc3QgPSBldmVudC5hcmd1bWVudHMo KVswXQorCisgICAgICAgIG5pY2tuYW1lX2xlbmd0aCA9IGxlbihzZWxmLmNvbm5lY3Rpb24uZ2V0 X25pY2tuYW1lKCkpCisgICAgICAgIGlmIG5vdCBpcmNsaWIuaXJjX2xvd2VyKHJlcXVlc3RbOm5p Y2tuYW1lX2xlbmd0aF0pLnN0YXJ0c3dpdGgoaXJjbGliLmlyY19sb3dlcihzZWxmLmNvbm5lY3Rp b24uZ2V0X25pY2tuYW1lKCkpKToKKyAgICAgICAgICAgIHJldHVybgorCisgICAgICAgICMgU29t ZSBJUkMgY2xpZW50cywgbGlrZSB4Y2hhdC1nbm9tZSwgZGVmYXVsdCB0byB1c2luZyBhIGNvbW1h CisgICAgICAgICMgd2hlbiBhZGRyZXNzaW5nIHNvbWVvbmUuCisgICAgICAgIGlmIHJlcXVlc3Rb bmlja25hbWVfbGVuZ3RoXSA9PSAnOic6CisgICAgICAgICAgICByZXF1ZXN0ID0gcmVxdWVzdC5z cGxpdCgnOicpCisgICAgICAgIGVsaWYgcmVxdWVzdFtuaWNrbmFtZV9sZW5ndGhdID09ICcsJzoK KyAgICAgICAgICAgIHJlcXVlc3QgPSByZXF1ZXN0LnNwbGl0KCcsJykKKyAgICAgICAgZWxzZToK KyAgICAgICAgICAgIHJldHVybgorCisgICAgICAgIGlmIGxlbihyZXF1ZXN0KSA+IDE6CiAgICAg ICAgICAgICByZXNwb25zZSA9IHNlbGYuX2RlbGVnYXRlLmlyY19tZXNzYWdlX3JlY2VpdmVkKG5p Y2ssIHJlcXVlc3RbMV0pCiAgICAgICAgICAgICBpZiByZXNwb25zZToKICAgICAgICAgICAgICAg ICBjb25uZWN0aW9uLnByaXZtc2coc2VsZi5fY2hhbm5lbCwgcmVzcG9uc2UpCg== 131941 2012-03-14 15:40:12 -0700 2012-03-14 15:53:12 -0700 Patch bug-81101-20120314194011.patch text/plain 2144 gustavo U3VidmVyc2lvbiBSZXZpc2lvbjogMTEwNTYxCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggMmZlZjYzMDJlOTA1NDUzZTVlNDU0N2JiMTFiYWNjM2Ez M2U2NjM3My4uODc1NjQyMzBjNWRkNjY2OTIxNWRhODViMDJjMTU1YzU4ZjZiMjZkMCAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEz IEBACisyMDEyLTAzLTE0ICBHdXN0YXZvIE5vcm9uaGEgU2lsdmEgIDxnbnNAZ25vbWUub3JnPgor CisgICAgICAgIHNoZXJpZmZib3Qgc2hvdWxkIGFsc28gYmUgYWRkcmVzc2FibGUgd2l0aCBhIGNv bW1hIGluIGFkZGl0aW9uIHRvIGNvbG9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD04MTEwMQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9jb21tb24vbmV0L2lyYy9pcmNib3Qu cHk6CisgICAgICAgIChJUkNCb3Qub25fcHVibXNnKToKKwogMjAxMi0wMy0xMiAgT2phbiBWYWZh aSAgPG9qYW5AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIHdlYmtpdC1wYXRjaCBvcHRpbWl6ZS1i YXNlbGluZXMgc29tZXRpbWVzIGNyZWF0ZXMgYSBtYWMtZnV0dXJlIHJlc3VsdApkaWZmIC0tZ2l0 IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vbmV0L2lyYy9pcmNib3QucHkgYi9Ub29s cy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9uZXQvaXJjL2lyY2JvdC5weQppbmRleCAwNjFhNDNj ZjYyYjg2YTAyNzhjNmM1ODg4ZDY1MTUzMjcxODYzZDI0Li4zNGE1NjBjMTYyZjU3NjZmNjBiYmM1 MGU3ODFkMDk5ODQ4NGM1YmY1IDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2Nv bW1vbi9uZXQvaXJjL2lyY2JvdC5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1v bi9uZXQvaXJjL2lyY2JvdC5weQpAQCAtNzMsOCArNzMsMjYgQEAgY2xhc3MgSVJDQm90KGlyY2Jv dC5TaW5nbGVTZXJ2ZXJJUkNCb3QsIE1lc3NhZ2VQdW1wRGVsZWdhdGUpOgogCiAgICAgZGVmIG9u X3B1Ym1zZyhzZWxmLCBjb25uZWN0aW9uLCBldmVudCk6CiAgICAgICAgIG5pY2sgPSBpcmNsaWIu bm1fdG9fbihldmVudC5zb3VyY2UoKSkKLSAgICAgICAgcmVxdWVzdCA9IGV2ZW50LmFyZ3VtZW50 cygpWzBdLnNwbGl0KCI6IiwgMSkKLSAgICAgICAgaWYgbGVuKHJlcXVlc3QpID4gMSBhbmQgaXJj bGliLmlyY19sb3dlcihyZXF1ZXN0WzBdKSA9PSBpcmNsaWIuaXJjX2xvd2VyKHNlbGYuY29ubmVj dGlvbi5nZXRfbmlja25hbWUoKSk6CisgICAgICAgIHJlcXVlc3QgPSBldmVudC5hcmd1bWVudHMo KVswXQorCisgICAgICAgIGlmIG5vdCBpcmNsaWIuaXJjX2xvd2VyKHJlcXVlc3QpLnN0YXJ0c3dp dGgoaXJjbGliLmlyY19sb3dlcihjb25uZWN0aW9uLmdldF9uaWNrbmFtZSgpKSk6CisgICAgICAg ICAgICByZXR1cm4KKworICAgICAgICAjIFNvbWUgSVJDIGNsaWVudHMsIGxpa2UgeGNoYXQtZ25v bWUsIGRlZmF1bHQgdG8gdXNpbmcgYSBjb21tYQorICAgICAgICAjIHdoZW4gYWRkcmVzc2luZyBz b21lb25lLgorICAgICAgICB0cnk6CisgICAgICAgICAgICB2b2NhdGl2ZV9zZXBhcmF0b3IgPSBy ZXF1ZXN0W2xlbihjb25uZWN0aW9uLmdldF9uaWNrbmFtZSgpKV0KKyAgICAgICAgZXhjZXB0IElu ZGV4RXJyb3I6CisgICAgICAgICAgICByZXR1cm4KKworICAgICAgICBpZiB2b2NhdGl2ZV9zZXBh cmF0b3IgPT0gJzonOgorICAgICAgICAgICAgcmVxdWVzdCA9IHJlcXVlc3Quc3BsaXQoJzonLCAx KQorICAgICAgICBlbGlmIHZvY2F0aXZlX3NlcGFyYXRvciA9PSAnLCc6CisgICAgICAgICAgICBy ZXF1ZXN0ID0gcmVxdWVzdC5zcGxpdCgnLCcsIDEpCisgICAgICAgIGVsc2U6CisgICAgICAgICAg ICByZXR1cm4KKworICAgICAgICBpZiBsZW4ocmVxdWVzdCkgPiAxOgogICAgICAgICAgICAgcmVz cG9uc2UgPSBzZWxmLl9kZWxlZ2F0ZS5pcmNfbWVzc2FnZV9yZWNlaXZlZChuaWNrLCByZXF1ZXN0 WzFdKQogICAgICAgICAgICAgaWYgcmVzcG9uc2U6CiAgICAgICAgICAgICAgICAgY29ubmVjdGlv bi5wcml2bXNnKHNlbGYuX2NoYW5uZWwsIHJlc3BvbnNlKQo=