-
Notifications
You must be signed in to change notification settings - Fork 681
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.
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
Handle commands in a better way #154
Conversation
text = "/say " + text; | ||
text = "say " + text; | ||
} else { | ||
text = text.replace(/^\//, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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! 🙌
Please hold on before merging, I'd like to take second review :-) |
"action", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@astorije This is for you. You're right it's silly to trim the start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool, thanks!
👍 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.
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 :-)
There was a problem hiding this comment.
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.
👍 and merging. |
Handle commands in a better way
msg
as the first command, and any time you send a message, it won't try to execute any other command/ns
,/cs
,/hs
aliases, handled properly by the server now, fixes Don't intercept /(c|h|n)s shortcut commands #153/
by escaping it as//
(or more slashes)