shizmob/pydle

Support for `monitor` not fully detected

Closed this issue · 4 comments

ZeroKnight commented

In the discussion for #142, I noticed a small bug with monitor and unmonitor. Pydle checks for monitor support during capability negotiation, but not in RPL_ISUPPORT, which the IRCv3 docs currently state is where it should be indicated:

Support of this specification is indicated by the MONITOR token in RPL_ISUPPORT (005). This token takes an optional parameter, of the maximum amount of targets a client may have in their monitor list. If no parameter is specified, there is no limit. A typical token would be:

MONITOR=100

To wit, both irc.esper.net and irc.freenode.net support monitor and indicate as such with a MONITOR key in RPL_ISUPPORT, but do not include monitor in their CAP LS response. Currently, Pydle incorrectly determines that monitor is not supported on these networks, and as a result attempting to monitor or unmonitor a target will always return False.

I'm not familiar with the entire history of IRCv3's changes, but I'm assuming that perhaps at one point monitor was indicated in capability negotiation. However, that doesn't seem to be the case now, and gleaning support based on RPL_ISUPPORT seems to be the correct way.

I can create a pull request to address this, but I'd like to know how it should be done. I'm thinking along the lines of:

  • Adding an on_isupport_monitor callback and add to self._capabilities there.
  • Remove the check for monitor support during capability negotiation, e.g. base ircv3.monitor.MonitoringSupport off a different parent class?
    • I'm unsure whether or not this correctly adheres to the spec as it currently stands.
    • Could always keep it around in addition to the on_isupport_monitor callback.
      • The fact that RPL_ISUPPORT comes after capability negotiation would ensure monitor support is detected and set in Pydle.

Thoughts?

ZeroKnight commented

I've started a branch with some initial work toward this, along with a fix for a couple of bugs I noticed. I've abstained from including the change to use the up-to-date coroutine syntax on the (un)monitor methods until that change is greenlit in #142. It'll be easy enough to rebase, though.

develop...ZeroKnight:fix-143

theunkn0wn1 commented

Hm, I was under the impression IRCv3 was feature complete, but iirc wasn't completely stable at time of implementation.
I am also not familiar with the history on IRCv3, but IRCv3.2 clearly states monitor is in the RPL_ISUPPORT as you mentioned above. Absent of contradicting literature, looks like Pydle's implementation is simply wrong as per IRCv3.2

I'm unsure whether or not this correctly adheres to the spec as it currently stands.

Not sure either, I did go looking for contrary literature that supports the current implementation but came up empty.

e.g. base ircv3.monitor.MonitoringSupport off a different parent class?

Looks like pydle.features.ircv3.monitor.MonitoringSupport is inherited by pydle.features.ircv3.ircv3_2.IRCv3_2Support, which inherits from other features that inherit pydle.features.ircv3.cap.CapabilityNegotiationSupport; changing just MonitoringSupport to inherit from pydle.features.rfc1459.client.RFC1459Support shouldn't cause any regressions.

Looks like the correct approach would be to hook into ISUPPORT, possibly deriving pydle.features.isupport.ISUPPORTSupport

In #142 an implementation error in monitor was noted, which contributes to this bug as returning True or False is irrelevant if the monitor messages are never emitted.

Looking at pydle/features/ircv3/monitor.py im noticing several errors in file that will need to be corrected, namely nickname being used in place of nick in on_raw_730 and on_raw_731. The monitor API will also need to be made async, and the call to self.rawmsg needs to be awaited.

ZeroKnight commented

e.g. base ircv3.monitor.MonitoringSupport off a different parent class?

Looks like pydle.features.ircv3.monitor.MonitoringSupport is inherited by pydle.features.ircv3.ircv3_2.IRCv3_2Support, which inherits from other features that inherit pydle.features.ircv3.cap.CapabilityNegotiationSupport; changing just MonitoringSupport to inherit from pydle.features.rfc1459.client.RFC1459Support shouldn't cause any regressions.

Looks like the correct approach would be to hook into ISUPPORT, possibly deriving pydle.features.isupport.ISUPPORTSupport

That sounds like the right course to me.

In #142 an implementation error in monitor was noted, which contributes to this bug as returning True or False is irrelevant if the monitor messages are never emitted.

Yeah, thinking about it, should these even return anything? I don't think other commands do this, so if someone tries to monitor on a server that doesn't support it, the unknown command handler should kick in.

Looking at pydle/features/ircv3/monitor.py im noticing several errors in file that will need to be corrected, namely nickname being used in place of nick in on_raw_730 and on_raw_731. The monitor API will also need to be made async, and the call to self.rawmsg needs to be awaited.

Yeah, I've been fixing the bugs there and working on this on the branch that I linked here. I've gone ahead and converted (un)monitor to proper coroutines and changed them to check for MONITOR in self._isupport instead. These are pushed if you'd like to take a look. I'm going to change its parent class now and make sure nothing catches fire before pushing that as well.

theunkn0wn1 commented

Closed by #144