Adding support for command rpl_whoreply (352)

[lan17]

I found that this library lacks support for this command, which my ZNC bouncer seems to send me a lot.

I have implemented support for it and have tested it by using it within my own project which uses this library.

As a special case I have emitted whois information right away because the end command - rpl_endofwho lacks nick so its impossible to match it with original rpl_whoreply

Let me know what you guys think.

[moshmage]

I would have avoided the declaration of variables in there and use its counterpart value (lets not forget this is a listener, which will iterate to infinity) apart from that nitpicking, 👍

[lan17]

moshmage, should I declare it at the top of the file?

[moshmage]

@lan17 nay, my nitpicking was "do not use variables as aliases" so, the code would become something like

case 'rpl_whoreply':
    self._addWhoisData(message.args[5], 'user', message.args[2]);
    self._addWhoisData(message.args[5], 'host', message.args[3]);
    self._addWhoisData(message.args[5], 'server', message.args[4]);
    self._addWhoisData(message.args[5], 'realname', message.args[7].match(/[0-9]+\s*(.+)/g)[1]);
    // emit right away because rpl_endofwho doesn't contain nick
    self.emit('whois', self._clearWhoisData(message.args[5]));
    break;

Would the code be harder to read? Probably, as we have to keep track that message.args[5] is the users' nickname. However, message.args[5] is, almost, always the nickname (looking at you, case 'NICK').

You could also use the self.nick instead of message.args[5] as that property is set on nick-change (or nick-set) - This could also be used for the "rpl_endofwho doesn't contain nick" problem. As we know self.nick is the users nickname we can safely use that on the rpl_endofwho case (am I right, @jirwin ?)

[lan17]

It seems that rpl_endofwho is not guaranteed to be paired with a single rpl_whoreply.

I see behavior (from a ZNC bouncer) where there are a bunch of rpl_whoreply that are followed by a single rpl_endofwho. Therefore I don't think we can rely on rpl_endofwho and should rather emit right away.

[lan17]

I have updated this pull request with @moshmage comments, however I did not add support for "rpl_endofwho" for reasons stated above.

[moshmage]

👍

Later on we'll have to whip some way of connecting rpl_endofwho and its starting counterpart - shouldn't be too hard. I have something in mind.

[lan17]

Awesome. Keep in mind, however, that its impossible to pair rpl_whoreply with rpl_endofwho because its many to one relationship (many rpl_whoreply for single rpl_endofwho at least with ZNC).

Perhaps instead we can have a state machine where if we are receiving rpl_whoreply and we have another one being prepared then we flush previous one.

[lan17]

What are the next steps? I need approval by martynsmith in order to merge?

[jirwin]

+1

[moshmage]

+1