Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle commands in a better way #154

Merged
merged 2 commits into from Mar 11, 2016
Merged

Handle commands in a better way #154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42a7c54
Fix #153: Do not intercept service commands
xPaw Mar 6, 2016
842b8d6
Send unknown commands to the server
xPaw Mar 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 19 additions & 9 deletions src/client.js
View file
Open in desktop
Expand Up @@ -33,21 +33,21 @@ var events = [
"whois"
];
var inputs = [
// These inputs are sorted in order that is most likely to be used
"msg",
"whois",
"part",
"action",
"connect",
"invite",
"join",
"kick",
"mode",
"msg",
"nick",
"notice",
"part",
"quit",
"raw",
"services",
"topic",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a stupid question, so don't be too tough on me :-)

Rather than making assumptions on most frequently used commands and calling them one by one until we have a match, why don't we directly math the input to the command name?
From what I see, /join matches to join, /nick matches to nick, ...
If this is not 100% the case, we can either forward commands, rename them, or treat the non-matching one differently.

EDIT: Disregard this comment for the current PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put whois at the top because it's triggered by clicking on a username. Ideally though, yes, we would have a direct hashmap of the commands.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After getting that the point of this PR was to avoid a full rewrite, you can disregard this earlier comment.
I'll implement this in a further PR as well as include a bunch of tests for these at the same time, I should get to it within the next days.

By the way, I agree with this order, was just saying it could get opinionated at some point, but that won't matter when I add direct mapping.

"whois"
];

function Client(manager, name, config) {
Expand Down Expand Up @@ -270,16 +270,22 @@ Client.prototype.input = function(data) {
var client = this;
var text = data.text.trim();
var target = client.find(data.target);
if (text.charAt(0) !== "/") {
text = "/say " + text;

// This is either a normal message or a command escaped with a leading '/'
if (text.charAt(0) !== "/" || text.charAt(1) === "/") {
text = "say " + text.replace(/^\//, "");
} else {
text = text.substr(1);
}

var args = text.split(" ");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While testing this, I found an issue: any text starting with one or more whitespace does not send anything nor produce an error. It's because test here becomes ['', 'test'].
We can fix this with the first of the 2 solutions below. However, I wonder if it is any good or bad to keep multiple whitespaces within commands. For example, /join #channel becomes ['join', '', '', '#channel'] then back to join #channel... I don't know if that could be an issue in some scenarios, in which case the second solution below would solve it.

var args = text.trim().split(" ");
var args = text.split(" ").filter(function (a) { return a.length !== 0; });

I might be missing an alternative, but I wanted to address the bug in the first place and look for potential solutions :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted the trim change, it will have to go into the next overhaul.

var cmd = args.shift().replace("/", "").toLowerCase();
_.each(inputs, function(plugin) {
var cmd = args.shift().toLowerCase();

var result = inputs.some(function(plugin) {
try {
var path = "./plugins/inputs/" + plugin;
var fn = require(path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we load the requires for all inputs once and for all after we create the initial array L35?
Unless I am being mistaken, requireing all inputs at every command seems like an unnecessary overhead to me.

EDIT: Please disregard this comment for current PR.

fn.apply(client, [
return fn.apply(client, [
target.network,
target.chan,
cmd,
Expand All @@ -289,6 +295,10 @@ Client.prototype.input = function(data) {
console.log(path + ": " + e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you consider my comment above, this catch should be moved along with the requires outside of the loop execution, making the var result = inputs.some(...) even more direct.
By doing so, you also make sure that any bug coming from a discrepancy between the input list and the available files in the plugins folder is detected as soon as The Lounge is started, and not whenever someone uses the buggy command.
Also, the current code silently absorbs the error without re-throwing it. All plugins are (at least at the moment) coming from us, so having the app crash miserably is acceptable here: there is no point in providing a recovery state from there IMO.

The current code is a bit worrying I think, because it fairly difficult to detect a silenced bug as is, unless you keep an eye on the log (which, by the way, is not even the error log...).

EDIT: Please disregard this comment for current PR.

}
});

if (result !== true) {
target.network.irc.write(text);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! This is the magic sauce that will let things like /znc work! 🙌

}
};

Client.prototype.more = function(data) {
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/inputs/action.js
View file
Open in desktop
Expand Up @@ -26,4 +26,6 @@ module.exports = function(network, chan, cmd, args) {
});
break;
}

return true;
};
3 changes: 3 additions & 0 deletions src/plugins/inputs/connect.js
View file
Open in desktop
Expand Up @@ -2,10 +2,13 @@ module.exports = function(network, chan, cmd, args) {
if (cmd !== "connect" && cmd !== "server") {
return;
}

if (args.length !== 0) {
var client = this;
client.connect({
host: args[0]
});
}

return true;
};
2 changes: 2 additions & 0 deletions src/plugins/inputs/invite.js
View file
Open in desktop
Expand Up @@ -10,4 +10,6 @@ module.exports = function(network, chan, cmd, args) {
} else if (args.length === 1 && chan.type === "channel") {
irc.invite(args[0], chan.name); // Current channel
}

return true;
};
3 changes: 3 additions & 0 deletions src/plugins/inputs/join.js
View file
Open in desktop
Expand Up @@ -2,8 +2,11 @@ module.exports = function(network, chan, cmd, args) {
if (cmd !== "join") {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing return false; here?

Edit: It might be that you rely on the fact that return; is similar to return undefined; and that therefore is considered as not true, but since we return booleans everywhere now, I would be consistent in types. Also, false != undefined so this is less error prone in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter since both !undefined and !false return true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does if for any reason we add code checking == false, as undefined evaluates to false but is not false. My main issue is that this doesn't keep consistency in returned types, but I won't strongly oppose since this code will evolve anyway.

}

if (args.length !== 0) {
var irc = network.irc;
irc.join(args[0], args[1]);
}

return true;
};
3 changes: 3 additions & 0 deletions src/plugins/inputs/kick.js
View file
Open in desktop
Expand Up @@ -2,8 +2,11 @@ module.exports = function(network, chan, cmd, args) {
if (cmd !== "kick") {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing return false; here?

}

if (args.length !== 0) {
var irc = network.irc;
irc.kick(chan.name, args[0]);
}

return true;
};
4 changes: 3 additions & 1 deletion src/plugins/inputs/mode.js
View file
Open in desktop
Expand Up @@ -16,7 +16,7 @@ module.exports = function(network, chan, cmd, args) {
"devoice": "-v"
}[cmd];
} else if (args.length === 1) {
return;
return true;
} else {
mode = args[0];
user = args[1];
Expand All @@ -27,4 +27,6 @@ module.exports = function(network, chan, cmd, args) {
mode,
user
);

return true;
};
6 changes: 4 additions & 2 deletions src/plugins/inputs/msg.js
View file
Open in desktop
Expand Up @@ -5,14 +5,14 @@ module.exports = function(network, chan, cmd, args) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing return false; here?

Edit: silencing for the remaining ones. If I'm wrong, I don't need to advertise it n times :D

}
if (args.length === 0 || args[0] === "") {
return;
return true;
}
var irc = network.irc;
var target = "";
if (cmd === "msg") {
target = args.shift();
if (args.length === 0) {
return;
return true;
}
} else {
target = chan.name;
Expand All @@ -27,4 +27,6 @@ module.exports = function(network, chan, cmd, args) {
message: msg
});
}

return true;
};
3 changes: 3 additions & 0 deletions src/plugins/inputs/nick.js
View file
Open in desktop
Expand Up @@ -2,8 +2,11 @@ module.exports = function(network, chan, cmd, args) {
if (cmd !== "nick") {
return;
}

if (args.length !== 0) {
var irc = network.irc;
irc.nick(args[0]);
}

return true;
};
2 changes: 2 additions & 0 deletions src/plugins/inputs/notice.js
View file
Open in desktop
Expand Up @@ -27,4 +27,6 @@ module.exports = function(network, chan, cmd, args) {
chan: targetChan.id,
msg: msg
});

return true;
};
4 changes: 4 additions & 0 deletions src/plugins/inputs/part.js
View file
Open in desktop
Expand Up @@ -4,15 +4,19 @@ module.exports = function(network, chan, cmd, args) {
if (cmd !== "part" && cmd !== "leave" && cmd !== "close") {
return;
}

if (chan.type !== "query") {
var irc = network.irc;
if (args.length === 0) {
args.push(chan.name);
}
irc.part(args);
}

network.channels = _.without(network.channels, chan);
this.emit("part", {
chan: chan.id
});

return true;
};
2 changes: 2 additions & 0 deletions src/plugins/inputs/quit.js
View file
Open in desktop
Expand Up @@ -16,4 +16,6 @@ module.exports = function(network, chan, cmd, args) {
});

irc.quit(quitMessage);

return true;
};
3 changes: 3 additions & 0 deletions src/plugins/inputs/raw.js
View file
Open in desktop
Expand Up @@ -2,8 +2,11 @@ module.exports = function(network, chan, cmd, args) {
if (cmd !== "raw" && cmd !== "send" && cmd !== "quote") {
return;
}

if (args.length !== 0) {
var irc = network.irc;
irc.write(args.join(" "));
}

return true;
};
26 changes: 0 additions & 26 deletions src/plugins/inputs/services.js
View file
Open in desktop

This file was deleted.

2 changes: 2 additions & 0 deletions src/plugins/inputs/topic.js
View file
Open in desktop
Expand Up @@ -9,4 +9,6 @@ module.exports = function(network, chan, cmd, args) {

var irc = network.irc;
irc.write(msg);

return true;
};
3 changes: 3 additions & 0 deletions src/plugins/inputs/whois.js
View file
Open in desktop
Expand Up @@ -2,8 +2,11 @@ module.exports = function(network, chan, cmd, args) {
if (cmd !== "whois" && cmd !== "query") {
return;
}

if (args.length !== 0) {
var irc = network.irc;
irc.whois(args[0]);
}

return true;
};