Handle commands in a better way #154
Conversation
xPaw
added
the
Type: Feature
| text = "/say " + text; | ||
| text = "say " + text; | ||
| } else { | ||
| text = text.replace(/^\//, ""); |
There was a problem hiding this comment.
Couldn't you have used substr instead of a regex to strip the first char?
|
I know there was just a release yesterday, but I'm already excited for the next one because it'll let me use |
|
Looks good and works fine for me 👍 Pretty good way to fix it without a rewrite. |
| } catch (e) { | ||
| console.log(path + ": " + e); | ||
| } | ||
| }); | ||
|
|
||
| if (result !== true) { | ||
| target.network.irc.write(text); |
There was a problem hiding this comment.
Yes! This is the magic sauce that will let things like /znc work! 🙌
|
Please hold on before merging, I'd like to take second review :-) |
| "action", | ||
|
|
There was a problem hiding this comment.
Can you remove this blank line?
|
@xPaw, I left you a few comments inline. This is globally great work and I am eager to see what it looks like when you address the comments (assuming I am not mistaken on them). |
|
Alright @xPaw, I added a note on all comments that you can ignore for this PR, keeping them around for archive purpose. Left the others untouched. |
|
@astorije Fixed some of the things you pointed out except for the |
| @@ -268,18 +268,28 @@ Client.prototype.setPassword = function(hash) { | |||
|
|
|||
| Client.prototype.input = function(data) { | |||
| var client = this; | |||
| var text = data.text.trim(); | |||
| var text = data.text.replace(/\s+$/, ""); | |||
There was a problem hiding this comment.
@astorije This is for you. You're right it's silly to trim the start.
|
👍 but I will test locally before merging tonight if you don't mind, @xPaw. |
| } | ||
|
|
||
| var args = text.split(" "); |
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
I've reverted the trim change, it will have to go into the next overhaul.
|
👍 and merging. |
Handle commands in a better way
msgas the first command, and any time you send a message, it won't try to execute any other command/ns,/cs,/hsaliases, handled properly by the server now, fixes Don't intercept /(c|h|n)s shortcut commands #153/by escaping it as//(or more slashes)