Closed ZeroKnight closed 2 years ago
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.
https://github.com/Shizmob/pydle/compare/develop...ZeroKnight:fix-143
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.
e.g. base
ircv3.monitor.MonitoringSupport
off a different parent class?Looks like
pydle.features.ircv3.monitor.MonitoringSupport
is inherited bypydle.features.ircv3.ircv3_2.IRCv3_2Support
, which inherits from other features that inheritpydle.features.ircv3.cap.CapabilityNegotiationSupport
; changing justMonitoringSupport
to inherit frompydle.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, namelynickname
being used in place ofnick
inon_raw_730
andon_raw_731
. The monitor API will also need to be made async, and the call toself.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.
Closed by #144
In the discussion for #142, I noticed a small bug with
monitor
andunmonitor
. Pydle checks formonitor
support during capability negotiation, but not inRPL_ISUPPORT
, which the IRCv3 docs currently state is where it should be indicated:To wit, both irc.esper.net and irc.freenode.net support
monitor
and indicate as such with aMONITOR
key inRPL_ISUPPORT
, but do not includemonitor
in theirCAP LS
response. Currently, Pydle incorrectly determines thatmonitor
is not supported on these networks, and as a result attempting tomonitor
orunmonitor
a target will always returnFalse
.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 onRPL_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:
on_isupport_monitor
callback and add toself._capabilities
there.monitor
support during capability negotiation, e.g. baseircv3.monitor.MonitoringSupport
off a different parent class?on_isupport_monitor
callback.RPL_ISUPPORT
comes after capability negotiation would ensuremonitor
support is detected and set in Pydle.Thoughts?