Hacky fix for missing quit messages when spam control was ON #100

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@valscion

This fixes #89

@damianb

Why not add a the functionality to purge the queue and then leverage that? Could be useful in other ways too.

@valscion

All I did was hack a bit, to show why it fails. Maybe this pull request should not be merged but rather be an aid for the original developer to fix it.

@valscion valscion commented on the diff Aug 13, 2012
lib/irc.js
@@ -781,9 +790,9 @@ function parseMessage(line, stripColors) { // {{{
// Parse parameters
if ( line.indexOf(':') != -1 ) {
- match = line.match(/(.*)(?:^:|\s+:)(.*)/);
- middle = match[1].trimRight();
- trailing = match[2];
+ var index = line.indexOf(':');
+ middle = line.substr(0, index).replace(/ +$/, "");
+ trailing = line.substr(index+1);
@valscion
valscion Aug 13, 2012

For the lines above, @tuhoojabotti might have something to say. This part of the pull request is not made by me.

@tuhoojabotti
tuhoojabotti Aug 16, 2012

I don't recollect doing that code, maybe it's probably a change on master that is not yet in the NPM package or something.

@valscion
valscion Aug 16, 2012

Oh, it was made by pull rq #91. Perhaps I should merge the master branch of node-irc to my fork...

@martynsmith
Owner

Yeah, I can see why this would be annoying.

I'll try investigate a nice way to empty the queue prior to disconnect.

In general are people happy to simply discard everything except the QUIT, send the QUIT and then disconnect, or should the client process the queue normally then disconnect?

At a guess I suspect the latter behaviour is more desirable.

@ndrnrd

Do agree with you: the latter is better. I'm experiencing this problem too on QuakeNet.

@valscion

👍 for the latter. I doubt that the queue would ever be too big to drain when client is exiting.

@tuhoojabotti

It could be a parameter for the quit function. Like if the bot breaks and starts spamming you can force quit.

@valscion

I like what @tuhoojabotti suggested. Do that.

@martynsmith
Owner

I'll close this (I've made a new issue #138 to keep track of the fact this needs fixing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment