archive/phabricator
1
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

First pass at decoupling Phabricator bot behavior from the protocol it's running on, this pulls the connection, reading, and writing functionalities out of the bot itself and into the adapter.

Browse Source 
Summary:
Ugh, just wrote out a huge message, only to lose it with a fat-fingered ctrl-c. Le sigh.

First pass at decoupling the bot from the protocol. Noticeably absent is the command/message coupling. After this design pass I'll give that a go. Could use some advice, thinking that handlers should only create messages (which can be public or private) and not open ended, undefined 'commands'. The problem being that there needs to be some consistant api if we want handlers to be protocol agnostic. Perhaps that's a pipedream, what are your thoughts?

Secondly, a few notes, design review requests on the changes i did make:
 # Config. For now i'm passing config through to the adapter. This was mainly to remain backwards compatible on the config. I was thinking it should probably be namespaced into it's own subobject though to distinguish the adapter config from the bot config.
 # Adapter selection. This flavor is the one-bot-daemon, config specified protocol version. The upside is that in the future they won't have to run different daemons for this stuff, just have different config, and the door is open for multiple protocol adapters down the road if need be. The downside is that I had to rename the daemon (non-backwards compatible change) and there will need to be some sort of runtime evaluation for instatiation of the adapter. For now I just have a crude switch, but I was thinking of just taking the string they supply as the class name (ala `try { new $clasName(); } catch...`) so as to allow for homegrown adapters, but I wasn't sure how such runtime magic would go over. Also, an alternative would be to make the PhabricatorBot class a non-abstract non-final base class and have the adapters be accompanied by a bot class that just defines their adapter as a property. The upside of which is backwards compatibility (welcome back PhabricatorIRCBot) and perhaps a little bit clearer plugin path for homegrowners.
 # Logging. You'll notice I commented out two very important logging lines in the irc adapter. This isn't intended to remain commented out, but I'm not sure what the best way is to get logging at this layer. I'm wary of just composing the daemon back down into the adapter (bi-directional object composition makes my skin crawl), but something needs to happen, obviously. Advice?

That's it. After the feedback on the above, you can either merge down, or wait until i finish the command/message refactor if you don't think the diff will grow too large. Up to you, this all functions as is.

Test Plan: Ran an irc bot, connected, read input, and wrote output including handler integration.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2462

Differential Revision: https://secure.phabricator.com/D4757
This commit is contained in:
indiefan
2013-02-05 18:45:20 -08:00
committed by epriestley
parent cb38ab27ce
commit 574bc3ba31
18 changed files with 408 additions and 298 deletions
Download Patch File Download Diff File Expand all files Collapse all files

199
src/infrastructure/daemon/bot/handler/PhabricatorBotObjectNameHandler.php Normal file
View File

@@ -0,0 +1,199 @@
<?php
/**
* Looks for Dxxxx, Txxxx and links to them.
*
* @group irc
*/
final class PhabricatorBotObjectNameHandler extends PhabricatorBotHandler {
/**
* Map of PHIDs to the last mention of them (as an epoch timestamp); prevents
* us from spamming chat when a single object is discussed.
*/
private $recentlyMentioned = array();
public function receiveMessage(PhabricatorBotMessage $message) {
switch ($message->getCommand()) {
case 'PRIVMSG':
$reply_to = $message->getReplyTo();
if (!$reply_to) {
break;
}
$message = $message->getMessageText();
$matches = null;
$pattern =
'@'.
'(?<!/)(?:^|\b)'. // Negative lookbehind prevent matching "/D123".
'(D|T|P|V|F)(\d+)'.
'(?:\b|$)'.
'@';
$revision_ids = array();
$task_ids = array();
$paste_ids = array();
$commit_names = array();
$vote_ids = array();
$file_ids = array();
if (preg_match_all($pattern, $message, $matches, PREG_SET_ORDER)) {
foreach ($matches as $match) {
switch ($match[1]) {
case 'D':
$revision_ids[] = $match[2];
break;
case 'T':
$task_ids[] = $match[2];
break;
case 'P':
$paste_ids[] = $match[2];
break;
case 'V':
$vote_ids[] = $match[2];
break;
case 'F':
$file_ids[] = $match[2];
break;
}
}
}
$pattern =
'@'.
'(?<!/)(?:^|\b)'.
'(r[A-Z]+[0-9a-z]{1,40})'.
'(?:\b|$)'.
'@';
if (preg_match_all($pattern, $message, $matches, PREG_SET_ORDER)) {
foreach ($matches as $match) {
$commit_names[] = $match[1];
}
}
$output = array();
if ($revision_ids) {
$revisions = $this->getConduit()->callMethodSynchronous(
'differential.query',
array(
'ids' => $revision_ids,
));
$revisions = array_select_keys(
ipull($revisions, null, 'id'),
$revision_ids
);
foreach ($revisions as $revision) {
$output[$revision['phid']] =
'D'.$revision['id'].' '.$revision['title'].' - '.
$revision['uri'];
}
}
if ($task_ids) {
foreach ($task_ids as $task_id) {
if ($task_id == 1000) {
$output[1000] = 'T1000: A nanomorph mimetic poly-alloy'
.'(liquid metal) assassin controlled by Skynet: '
.'http://en.wikipedia.org/wiki/T-1000';
continue;
}
$task = $this->getConduit()->callMethodSynchronous(
'maniphest.info',
array(
'task_id' => $task_id,
));
$output[$task['phid']] = 'T'.$task['id'].': '.$task['title'].
' (Priority: '.$task['priority'].') - '.$task['uri'];
}
}
if ($vote_ids) {
foreach ($vote_ids as $vote_id) {
$vote = $this->getConduit()->callMethodSynchronous(
'slowvote.info',
array(
'poll_id' => $vote_id,
));
$output[$vote['phid']] = 'V'.$vote['id'].': '.$vote['question'].
' Come Vote '.$vote['uri'];
}
}
if ($file_ids) {
foreach ($file_ids as $file_id) {
$file = $this->getConduit()->callMethodSynchronous(
'file.info',
array(
'id' => $file_id,
));
$output[$file['phid']] = $file['objectName'].": ".$file['uri']." - ".
$file['name'];
}
}
if ($paste_ids) {
foreach ($paste_ids as $paste_id) {
$paste = $this->getConduit()->callMethodSynchronous(
'paste.info',
array(
'paste_id' => $paste_id,
));
// Eventually I'd like to show the username of the paster as well,
// however that will need something like a user.username_from_phid
// since we (ideally) want to keep the bot to Conduit calls...and
// not call to Phabricator-specific stuff (like actually loading
// the User object and fetching his/her username.)
$output[$paste['phid']] = 'P'.$paste['id'].': '.$paste['uri'].' - '.
$paste['title'];
if ($paste['language']) {
$output[$paste['phid']] .= ' ('.$paste['language'].')';
}
}
}
if ($commit_names) {
$commits = $this->getConduit()->callMethodSynchronous(
'diffusion.getcommits',
array(
'commits' => $commit_names,
));
foreach ($commits as $commit) {
if (isset($commit['error'])) {
continue;
}
$output[$commit['commitPHID']] = $commit['uri'];
}
}
foreach ($output as $phid => $description) {
// Don't mention the same object more than once every 10 minutes
// in public channels, so we avoid spamming the chat over and over
// again for discsussions of a specific revision, for example.
if (empty($this->recentlyMentioned[$reply_to])) {
$this->recentlyMentioned[$reply_to] = array();
}
$quiet_until = idx(
$this->recentlyMentioned[$reply_to],
$phid,
0) + (60 * 10);
if (time() < $quiet_until) {
// Remain quiet on this channel.
continue;
}
$this->recentlyMentioned[$reply_to][$phid] = time();
$this->write('PRIVMSG', "{$reply_to} :{$description}");
}
break;
}
}
}