[Twisted-Python] IRC protocol rewrite?
Hi All, I'm currently investigating the IRC Protocol as implemented by twisted, to see if I perhaps can use it in my irc client, or if it perhaps can use code from my irc protocol parsing. However, I noticed some bugs and (imho) design failures in the current implementation, which makes the code not very usefull for any other use than what it's currently used for (twisted irc client + server?) For example, channels are hardcoded as starting with '#', i.e. IRCClient.join prepends a '#' to the channelname. Hoewever, most servers support alernative style channels such as &channel (local channels), +channel (modeless channels), and perhaps more. Furthermore, IMHO, the IRCClient class implements too many protocols in one class. IRC, CTCP and DCC are distinct protocols, and each belong in their own class. Lastly, the current parsemsg routine can't handle ipv6 addresses in certain replies, i.e. a 311 (whois) reply like: :irc.xs4all.nl 311 VladDrac [p] -patrick 3ffe:2500:900:2000:1337:1337:1337:1337 *:foo will be parsed incorrectly. I may provide a patch for this later. Is replacing the current IRC protocol with a (non-api-compatible) new protocol discussable? If so, I'll make some suggestions on how I would implement the protocol. Cheers, Ivo -- Drs. I.R. van der Wijk -=- Brouwersgracht 132 Amaze Internet Services V.O.F. 1013 HA Amsterdam, NL -=- Tel: +31-20-4688336 Linux/Web/Zope/SQL/MMBase Fax: +31-20-4688337 Network Solutions Web: http://www.amaze.nl/ Consultancy Email: ivo@amaze.nl -=-
On Wed, 2002-01-02 at 11:46, Ivo van der Wijk wrote:
Hi All,
I'm currently investigating the IRC Protocol as implemented by twisted, to see if I perhaps can use it in my irc client, or if it perhaps can use code from my irc protocol parsing.
However, I noticed some bugs and (imho) design failures in the current implementation, which makes the code not very usefull for any other use than what it's currently used for (twisted irc client + server?)
For example, channels are hardcoded as starting with '#', i.e. IRCClient.join prepends a '#' to the channelname. Hoewever, most servers support alernative style channels such as &channel (local channels), +channel (modeless channels), and perhaps more.
This is an intentional design failure :). The feature-set of IRC is highly idiosyncratic, and difficult to represent in terms of "real" abstractions -- for example, the notion of a "modeless" channel doesn't particularly make sense outside of the world of IRC's implementation; there are no gradations between "modeful" and "modeless" channels, unless you have a Services implementation w/ ChanServ and mode lock. "local" channels don't either, considering that they are really just a way to introduce an artificial barrier between servers. The whole point of the R in IRC is to avoid that :) I have to agree that for the client, it's probably a bad idea to enforce restrictions like that, since it's designed to connect to other IRC servers, and be generally useful to connect to external services which may be set up in bizarre configurations. On the server, however, I would like to drop support for as many IRCisms as reasonably possible, so that we can implement a richer set of functionality; probably through the "services" idiom, since that allows for fairly arbitrary specification of inputs (as opposed to the limited set of client-understood modes that one may specify, and the 26 allowable characters).
Furthermore, IMHO, the IRCClient class implements too many protocols in one class. IRC, CTCP and DCC are distinct protocols, and each belong in their own class.
I disagree. IRC is a poorly specified protocol in its RFC, so the "usual" extensions are a part of any reasonable IRC implementation these days. Unless you can come up with a requirement for functionality that would require them being in different classes, I think they should stay where they are.
Lastly, the current parsemsg routine can't handle ipv6 addresses in certain replies, i.e. a 311 (whois) reply like:
:irc.xs4all.nl 311 VladDrac [p] -patrick 3ffe:2500:900:2000:1337:1337:1337:1337 *:foo
will be parsed incorrectly. I may provide a patch for this later.
Whoops. That definitely sounds like it needs to be fixed :)
Is replacing the current IRC protocol with a (non-api-compatible) new protocol discussable? If so, I'll make some suggestions on how I would implement the protocol.
Certainly discussable, especially since the IRC support is not as good as it could be. If you replace it with something non-API-compatible, I'd like to hear from the authors of code that this would break, and certainly refactor any code currently in Twisted to work the new way. -- ______ you are in a maze of twisted little applications, all | |_\ remarkably consistent. | | -- glyph lefkowitz, glyph @ twisted matrix . com |_____| http://www.twistedmatrix.com/
On Wed, 2002-01-02 at 09:46, Ivo van der Wijk wrote:
However, I noticed some bugs and (imho) design failures in the current implementation, which makes the code not very usefull for any other use than what it's currently used for (twisted irc client + server?)
(And Tendril. Don't forget Tendril, in twisted.words.tendril.) I'm interested in hearing what makes it "not very useful" though. I didn't really get a sense of that from your message.
For example, channels are hardcoded as starting with '#', i.e. IRCClient.join prepends a '#' to the channelname. Hoewever, most servers support alernative style channels such as &channel (local channels), +channel (modeless channels), and perhaps more.
Valid point. Of course, nobody actually uses those in real life anymore, but it'd be fine to support it in the interest of completeness.
Furthermore, IMHO, the IRCClient class implements too many protocols in one class. IRC, CTCP and DCC are distinct protocols, and each belong in their own class.
There's some sense to this too. As a Protocol class, IRCClient is overfeatured (as a "client", maybe it's okay). I'm not quite sure how to split off the CTCP stuff though, as intertwined as the CTCP messages are with IRC. Would you define IRCClient not only as a Protocol, but as a Transport for CTCP? Eaugh... A somewhat more mundane option would be to just put all the ctcp* methods in a mixin class. That might make the organization a little tidier, but it wouldn't change the interface at all, so I'm not sure if that would satisfy your concerns or not. As for DCC, it's in seperate protocol classes already.
Lastly, the current parsemsg routine can't handle ipv6 addresses in certain replies, i.e. a 311 (whois) reply like:
:irc.xs4all.nl 311 VladDrac [p] -patrick 3ffe:2500:900:2000:1337:1337:1337:1337 *:foo
will be parsed incorrectly. I may provide a patch for this later.
Eh? It's not immediately obvious to me what will break. But patches are always fine. =)
Is replacing the current IRC protocol with a (non-api-compatible) new protocol discussable? If so, I'll make some suggestions on how I would implement the protocol.
IMO, speaking as the guy who's probably spent the most time with it, it's certainly discussable. Unless there was some message I didn't read from the BDFL about an API freeze. ;) "Oh no, it's a trap!"-ly, - Kevin (Acapnotic) -- The moon is waning gibbous, 86.8% illuminated, 18.3 days old.
On Wed, Jan 02, 2002 at 02:38:00PM -0800, Kevin Turner wrote:
On Wed, 2002-01-02 at 09:46, Ivo van der Wijk wrote:
However, I noticed some bugs and (imho) design failures in the current implementation, which makes the code not very usefull for any other use than what it's currently used for (twisted irc client + server?)
(And Tendril. Don't forget Tendril, in twisted.words.tendril.) I'm interested in hearing what makes it "not very useful" though. I didn't really get a sense of that from your message.
The current implementation is rather specific. For example, it assumes only #-like channels exist, it takes actions/decisions the client builder would rather like to implement (i.e. irc_ERR_NICKNAMEINUSE will generate a new nickname by appending a _ - who says I want this in my client?), ctcp_PING immediately sends a ping reply - this won't work with my ignore list, etc. I'm not saying this is wrong behaviour, it just does not belong at this level in the protocol. I just want a basic abstraction of the server connecting/disconnecting, server messages receiving (dispatching) and sending. Anything else should be implemented in a derived/separate class.
For example, channels are hardcoded as starting with '#', i.e. IRCClient.join prepends a '#' to the channelname. Hoewever, most servers support alernative style channels such as &channel (local channels), +channel (modeless channels), and perhaps more.
Valid point. Of course, nobody actually uses those in real life anymore, but it'd be fine to support it in the interest of completeness.
IRCops tend to visit channels like &ERRORS and &NOTICES (at least ircnet supports this) to view server messages. Besides, you simply don't adhere to the 'protocol' (eventhough it's poorly documented). And we have MIRC for that, right? :)
Furthermore, IMHO, the IRCClient class implements too many protocols in one class. IRC, CTCP and DCC are distinct protocols, and each belong in their own class.
There's some sense to this too. As a Protocol class, IRCClient is overfeatured (as a "client", maybe it's okay). I'm not quite sure how to split off the CTCP stuff though, as intertwined as the CTCP messages are with IRC. Would you define IRCClient not only as a Protocol, but as a Transport for CTCP? Eaugh... A somewhat more mundane option would be to just put all the ctcp* methods in a mixin class. That might make the organization a little tidier, but it wouldn't change the interface at all, so I'm not sure if that would satisfy your concerns or not.
The name IRCClient is a bit misleading, but the basic IRC protocol *is* the transport for CTCP! CTCP is a separate protocol encoded in ^A's in PRIVMSG/NOTICE. Also, CTCP is way less documented than the irc protocol, and clients are (somewhat) free to make their own CTCP extension (i.e. you coud send whiteboard drawing commands over ctcp or communicate gameplay over ctcp over irc. Some clients (zircon?) actually do this)
As for DCC, it's in seperate protocol classes already.
There's is quite some DCC support in the IRCClient class that I have here (twisted 0.12.3), i.e. ctcpQuery_DCC which initiates DCC, maintains a list of sessions, etc
Lastly, the current parsemsg routine can't handle ipv6 addresses in certain replies, i.e. a 311 (whois) reply like:
:irc.xs4all.nl 311 VladDrac [p] -patrick 3ffe:2500:900:2000:1337:1337:1337:1337 *:foo
will be parsed incorrectly. I may provide a patch for this later.
Eh? It's not immediately obvious to me what will break. But patches are always fine. =)
parsemsg(":irc.xs4all.nl 311 VladDrac [p] -patrick 3ffe:2500:900:2000:1337:1337:1337:1337 *:foo") ('irc.xs4all.nl', '311', ['VladDrac', '[p]', '-patrick', '3ffe', '2500:900:2000:1337:1337:1337:1337 *:foo'])
It's the assumption that a ':' will start the remainder of the argument list, which is not true if one of the arguments is a ipv6 address.
Is replacing the current IRC protocol with a (non-api-compatible) new protocol discussable? If so, I'll make some suggestions on how I would implement the protocol.
IMO, speaking as the guy who's probably spent the most time with it, it's certainly discussable. Unless there was some message I didn't read from the BDFL about an API freeze. ;)
Ok :) Ivo -- Drs. I.R. van der Wijk -=- Brouwersgracht 132 Amaze Internet Services V.O.F. 1013 HA Amsterdam, NL -=- Tel: +31-20-4688336 Linux/Web/Zope/SQL/MMBase Fax: +31-20-4688337 Network Solutions Web: http://www.amaze.nl/ Consultancy Email: ivo@amaze.nl -=-
On Wed, 2002-01-02 at 15:18, Ivo van der Wijk wrote:
it takes actions/decisions the client builder would rather like to implement (i.e. irc_ERR_NICKNAMEINUSE will generate a new nickname by appending a _ - who says I want this in my client?), ctcp_PING immediately sends a ping reply - this won't work with my ignore list, etc.
Ah, this is the information I was looking for. (The bit about supporting & channels is a minor detail, worth fixing but not grounds for reimplementation.) In reply, let me just say that subclassing is a wonderful and powerful thing. You are invited, encouraged even, to override any methods which don't do what you want in your subclass. Default implementations are provided in the base class for things like irc_ERR_NICKNAMEINUSE because every IRC client needs to have *some* implementation of them, or else it will likely be so broken that it won't be able to sign on. But again, don't hesitate to override the default methods with something else. This class uses the """getattr(self, "prefix_%s" % command)()""" idiom, so you can always add methods for more IRC or CTCP message types (i.e. Zircon extensions) in your subclass too.
There's is quite some DCC support in the IRCClient class that I have here (twisted 0.12.3), i.e. ctcpQuery_DCC which initiates DCC, maintains a list of sessions, etc
That's because it's a CTCP query which initiates the DCC connection. The DCC connection itself (as opposed to things talking about DCC connections) has its own class and instance. As for maintaining a list of sessions, I think that this does belong in a "client" implementation, but I agree that this is not state that a "protocol" class should be concerned with. It's only there because I couldn't see another way for a subclass to figure out which DCC sessions were created, which is something you probably want to know for your interface. Perhaps the default ctcpQuery_DCC method should pass back the DCC session as its return value? -- The moon is waning gibbous, 86.3% illuminated, 18.3 days old.
On Wed, Jan 02, 2002 at 04:11:34PM -0800, Kevin Turner wrote:
On Wed, 2002-01-02 at 15:18, Ivo van der Wijk wrote:
it takes actions/decisions the client builder would rather like to implement (i.e. irc_ERR_NICKNAMEINUSE will generate a new nickname by appending a _ - who says I want this in my client?), ctcp_PING immediately sends a ping reply - this won't work with my ignore list, etc.
Ah, this is the information I was looking for. (The bit about supporting & channels is a minor detail, worth fixing but not grounds for reimplementation.) In reply, let me just say that subclassing is a wonderful and powerful thing. You are invited, encouraged even, to override any methods which don't do what you want in your subclass. Default implementations are provided in the base class for things like irc_ERR_NICKNAMEINUSE because every IRC client needs to have *some* implementation of them, or else it will likely be so broken that it won't be able to sign on. But again, don't hesitate to override the default methods with something else. This class uses the """getattr(self, "prefix_%s" % command)()""" idiom, so you can always add methods for more IRC or CTCP message types (i.e. Zircon extensions) in your subclass too.
Well, sure, I can override anything. But in this specific case I'd have to override so much (and stick with redundant unused code), that I'd rather re-implement things. Proposal: I will implement the protocol the way I see it for my irc client, somewhat based on the current code / the twisted philosophy (my client needs a rewrite in that area anyway), and if others think my code is leaner/better/whatever than the current, we may consider replacing the current irc protocol with it.
There's is quite some DCC support in the IRCClient class that I have here (twisted 0.12.3), i.e. ctcpQuery_DCC which initiates DCC, maintains a list of sessions, etc
That's because it's a CTCP query which initiates the DCC connection. The DCC connection itself (as opposed to things talking about DCC connections) has its own class and instance.
IMHO, this would be like an IP layer building (and maintaining) TCP sessions.
From the purists point of view, if I want the basic irc protocol (i.e. for some web messaging interface), I would have the redundant ctcp, dcc etc code as well. Sure, having it lingering and not using it around won't break anything, but the nice thing about OO is that you can encapsulate specific functionality into objects, extend them with extra (higher-level) features, etc.
As for maintaining a list of sessions, I think that this does belong in a "client" implementation, but I agree that this is not state that a "protocol" class should be concerned with. It's only there because I couldn't see another way for a subclass to figure out which DCC sessions were created, which is something you probably want to know for your interface. Perhaps the default ctcpQuery_DCC method should pass back the DCC session as its return value?
That would be better, yes. Cheers, Ivo -- Drs. I.R. van der Wijk -=- Brouwersgracht 132 Amaze Internet Services V.O.F. 1013 HA Amsterdam, NL -=- Tel: +31-20-4688336 Linux/Web/Zope/SQL/MMBase Fax: +31-20-4688337 Network Solutions Web: http://www.amaze.nl/ Consultancy Email: ivo@amaze.nl -=-
participants (3)
-
Glyph Lefkowitz -
Ivo van der Wijk -
Kevin Turner