Opened 12 years ago

Closed 10 years ago

#3285 enhancement closed fixed (fixed)

Add ISUPPORT implementation for irc.py

Reported by: Ezio Melotti Owned by: Jonathan Jacobs
Priority: high Milestone:
Component: words Keywords: irc IRCCLIENT ISUPPORT
Cc: Leonidas Branch: branches/isupport-3285-3
branch-diff, diff-cov, branch-cov, buildbot
Author: jonathanj, exarkun

Description

The method irc_RPL_BOUNCE (line 1182) detects the ISUPPORT message and sends it to the API function named isupport() (line 657) as it is. There should be instead a irc_RPL_ISUPPORT method that parses the string before sending it to isupport. Doing this will break the backward compatibility because the isupport() method is supposed to receive a string. The parsing could be done in the isupport() method too, but is not the right place and will led to another problem if the user overrides isupport() without calling the original isupport().

The ISUPPORT messagge is like:

:servername 005 mynick NOQUIT WATCH=128 SAFELIST MODES=6 MAXCHANNELS=15
MAXBANS=100 NICKLEN=30 TOPICLEN=307 KICKLEN=307 CHANTYPES=&# PREFIX=(ohv)@%+ 
NETWORK=networkname SILENCE=10 CASEMAPPING=ascii CHANMODES=b,k,l,cdijmMnOprRsStuU
:are available on this server

The ISUPPORT message defines a series of useful informations (like PREFIX and CHANMODES) that should be saved (at class or module level) and used in other functions. For example, the constant named CHANNEL_PREFIX is defined in irc.py (line 61) with the value '&#!+'. This constant is never used, but the value is hardcoded in other functions (in join, leave, kick, lines 825, 832, 839; in topic, say, me, line 855, 877, 941) always in the form "if channel[0] in '&#!+': channel = '#' + channel". This value is defined in the ISUPPORT message with the name CHANTYPES and could vary between servers so it should be saved and used (and also a function that adds the '#' before the channel should be done).

If the parsing is done in the isupport() method and the user overrides it, all these variables won't be set and although default variables can be used this could create problems if they differ from the ones sent by the server.

Another possible solution is to create the irc_RPL_ISUPPORT method that parses and save the informations (at class/module level) and change irc_RPL_BOUNCE to send the string both to this and to the isupport() method, so the informations will be saved and there won't be compatibility problems (unless the user already created an irc_RPL_ISUPPORT method). If the user wants to access this data could do it using directly the variables set at class/module level (isupport() will then be useless and we keep it just for compatibility).

If we implement this there will be to change several functions in order to use these values instead of hardcoded ones. The NAMES method (see ticket #3275) should also use the PREFIX values while parsing the list of users (that looks like @nick1 nick2 +nick3) in order to separate the prefixes from the nicks.

Attachments (12)

ticket3285.patch (7.5 KB) - added by Ezio Melotti 11 years ago.
ticket3285.2.patch (17.7 KB) - added by Ezio Melotti 11 years ago.
ticket3285.3.patch (19.8 KB) - added by Ezio Melotti 11 years ago.
ticket3285.4.patch (19.8 KB) - added by Ezio Melotti 11 years ago.
ticket3285.5.patch (19.8 KB) - added by Ezio Melotti 11 years ago.
ticket3285.6.patch (15.5 KB) - added by Ezio Melotti 11 years ago.
isupport-parser.diff (16.3 KB) - added by Jonathan Jacobs 11 years ago.
isupport-parser-1.diff (17.2 KB) - added by Jonathan Jacobs 10 years ago.
isupport-parser-2.diff (21.7 KB) - added by Jonathan Jacobs 10 years ago.
isupport-parser-3.diff (21.9 KB) - added by Jonathan Jacobs 10 years ago.
isupport-parser-4.diff (9.3 KB) - added by Jonathan Jacobs 10 years ago.
Created against isupport-3285-2 branch
isupport-parser-5.diff (6.5 KB) - added by Jonathan Jacobs 10 years ago.
Created against isupport-parser-4.diff applied to isupport-3285-2 branch.

Download all attachments as: .zip

Change History (77)

comment:1 Changed 12 years ago by Leonidas

Cc: Leonidas added

comment:2 Changed 12 years ago by Jean-Paul Calderone

author: Wolf

comment:3 Changed 12 years ago by Ezio Melotti

Priority: normalhigh

#3286 also needs the PREFIX value to parse the users.

comment:4 Changed 12 years ago by Jean-Paul Calderone

Owner: changed from Jean-Paul Calderone to Ezio Melotti

comment:5 Changed 11 years ago by Jonathan Jacobs

For what it's worth, UnrealIRCD appends its ISUPPORT lines with are supported by this server and not are available on this server as Twisted currently assumes.

comment:6 Changed 11 years ago by Ezio Melotti

I'm working on it, as suggested by glyph in #3286 I'll do a instance attribute for IRCClient. A message like

NOQUIT WATCH=128 SAFELIST MODES=6 MAXCHANNELS=15
MAXBANS=100 NICKLEN=30 TOPICLEN=307 KICKLEN=307 CHANTYPES=&# PREFIX=(ohv)@%+
NETWORK=networkname SILENCE=10 CASEMAPPING=ascii CHANMODES=b,k,l,cdijmMnOprRsStuU

will be parsed to:

self.ISUPPORT = {
  'FLAGS': set(['NOQUIT', 'SAFELIST']),
  'WATCH': 128,
  'MODES': 6,
  'PREFIX': '(ohv)@%+',
  'CHANMODES': 'b,k,l,cdijmMnOprRsStuU',
  ...

Some values, like PREFIX and CHANMODES will be parsed and the result will be saved in the same dict (e.g. PREFIX will result in two new keys: USERPREFIX and USERMODES - see #3286, comment 5), I'll do specific functions for that.

comment:7 Changed 11 years ago by Leonidas

I don't think there should be an distinction between FLAGS and the other informations. It it would be non-nested one could use if 'MODES' in self.ISUPPORT instead of if 'MODES' in self.ISUPPORT or 'MODES' in self.ISUPPORT['FLAGS'] which seems smarter. Remember the Zen of Python "Flat is better than nested". This would be useful, if there would be any name clashes of flags with keys, but I don't think there are any.

I'd also suggest something that is not ALL_UPPERCASE. ISUPPORT is probably not the smartest name, either. I'd love to see RPL_MYINFO (004) and RPL_ISUPPORT (005) merged into one dictionary. The distinction between these is as far as I see only historical - no client author would nowadays care whether the informations are from MYINFO or ISUPPORT.

comment:8 in reply to:  7 Changed 11 years ago by Ezio Melotti

Replying to Leonidas:

I don't think there should be an distinction between FLAGS and the other informations.

I don't think is a good thing having a key/value pair if we don't actually have any value and we only check if the key exists doing if mode in self.ISUPPORT, even if it's shorter to type.

... if there would be any name clashes of flags with keys, but I don't think there are any.

A lower-case 'flags' could solve this problem.

I'd also suggest something that is not ALL_UPPERCASE.

I used it for ISUPPORT because it should be a constant (once set), for the modes I just leave them as they are (and they are supposed to be constants too).

ISUPPORT is probably not the smartest name, either. I'd love to see RPL_MYINFO (004) and RPL_ISUPPORT (005) merged into one dictionary.

I agree but I don't know a better name, if you have something better that can describe the type of informations represented by 004 and 005 without being too long let me know.

Changed 11 years ago by Ezio Melotti

Attachment: ticket3285.patch added

comment:9 Changed 11 years ago by Ezio Melotti

I attached a patch so you can see if what I've done is ok (tests are not included here).

I did what I proposed the last time, so the dict is still named ISUPPORT (if we find a better name a I'll find&replace it) and the values (except 'flags') are all UPPERCASE. I haven't implemented MYINFO here.

I added a default ISUPPORT dict with some values for 2 reasons:

  1. If I create the dict when I receive the ISUPPORT message, and the server sends more ISUPPORT messages, only the last will be saved (even if I could check if hasattr(self, 'ISUPPORT') but I think is fine as it is).
  2. The methods can read or add keys from/to the dict without having to check if the dict or the keys exist.

This is also useful if the server doesn't send any ISUPPORT message, even if I'm not sure if it's better leave empty values, use default valuesas I did or create only an empty ISUPPORT dict and use ISUPPORT.get(key) every time. Also it would be nice to have some (all?) of these values accessible as instance-constants (i.e. self.CHANMODES instead of self.ISUPPORT['CHANMODES']) if they are widely used by the functions (also things like the network name have no reason to be in the dict).

Is probably better to use log.msg() instead of raise Exception because is not fault of the user if the server sends a wrong message (that should be just ignored).

Some parameters like PREFIX, CHANMODES, CHANLIMIT, MAXLIST, LANGUAGE (and possibly others) need further parsing. I only wrote methods to parse PREFIX and CHANMODES, the other methods could be easily added later, if ever someone will need them.

comment:10 in reply to:  9 ; Changed 11 years ago by Leonidas

Replying to Wolf:

I did what I proposed the last time, so the dict is still named ISUPPORT (if we find a better name a I'll find&replace it) and the values (except 'flags') are all UPPERCASE. I haven't implemented MYINFO here.

How about server_capab? It is not too long, while it does not imply that it is only ISUPPORT. I'm still sceptical about splitting the ISUPPORT into flags and non-flags. If they are in one dict, I can just query one dict whether or not some flag/param is set. Splitting it in two result in authors needing to think whether to query the server_capab dict or the set in server_capab['flags'] for no reason. I don't see where a dict key with FLAG : True would be bad. True implies that it is set. There are even more enhancements possible like adding all possible flags into server_capab and setting them to False per default and only setting these which are supported by the server to True. Not possible with a set, in that case the flags would need to move to the server_capab dict anyway. So the split is something artificial.

This is also useful if the server doesn't send any ISUPPORT message, even if I'm not sure if it's better leave empty values, use default valuesas I did or create only an empty ISUPPORT dict and use ISUPPORT.get(key) every time. Also it would be nice to have some (all?) of these values accessible as instance-constants (i.e. self.CHANMODES instead of self.ISUPPORT['CHANMODES']) if they are widely used by the functions (also things like the network name have no reason to be in the dict).

I wouldn't like the namespace of the class be cluttered with attributes that might or might not be set, especially when they are ALL UPPERCASE.

comment:11 in reply to:  10 ; Changed 11 years ago by Ezio Melotti

Replying to Leonidas:

How about server_capab? It is not too long, while it does not imply that it is only ISUPPORT.

This is surely better than ISUPPORT. I'll also remove the 'flags' key and I'll add the flags in the dict as normal keys, with True as value. (after all, "Special cases aren't special enough to break the rules." is followed by "Although practicality beats purity.")

I wouldn't like the namespace of the class be cluttered with attributes that might or might not be set, especially when they are ALL UPPERCASE.

I think that userprefix, usermodes and channelmodes (lowercase) can be set as instance attributes for two reasons:

  1. They are not really part of the original ISUPPORT message
  2. They are widely used by several functions, and it's better having them as handy attributes

(also the original version of irc.py has them as module-level constants, so a couple of instance-level attributes shouldn't be a problem)

I'm not sure what should I send to the iSupport method. All the parsed data will be already saved in the server_capab dict, so I can call it with no args, even if this could create problems if in future we need to send it some data, so maybe is better to pass to iSupport the self.server_capab dict

comment:12 in reply to:  11 ; Changed 11 years ago by Leonidas

Replying to Wolf:

I'm not sure what should I send to the iSupport method. All the parsed data will be already saved in the server_capab dict, so I can call it with no args, even if this could create problems if in future we need to send it some data, so maybe is better to pass to iSupport the self.server_capab dict

I'm not really sure about whether we need an iSupport method at all, when we save the flags into an instance attribute. But if you really want, you could parse the ISUPPORT message in irc_RPL_ISUPPORT, add the flags to server_capab and call self.iSupport(the_parsed_isupport). I don't think iSupport should ever get the MYINFO data, otherwise it would be quite pointless to have the server_capab attribute at all (and I think the attribute is a good idea).

comment:13 in reply to:  12 Changed 11 years ago by Ezio Melotti

Replying to Leonidas:

I'm not really sure about whether we need an iSupport method at all, when we save the flags into an instance attribute.

The user may want to know that we did it. I'll do self.server_capab = {} in connectionMade, and something like

the_parsed_isupport = parseISupport()
self.iSupport(the_parsed_isupport)
self.server_capab.update(the_parsed_isupport)

in irc_RPL_ISUPPORT, so the user will receive only the data from ISUPPORT (for the MYINFO message we can do something similar, saving all the data in server_capab and sending to myInfo only the parsed data).

comment:14 Changed 11 years ago by Ezio Melotti

The isupport method already exists and it was called passing a list of string, passing a dict will break the backward compatibility. A solution could be deprecate isupport and replace it with iSupport.

comment:15 Changed 11 years ago by Ezio Melotti

The isupport method will be deprecated and the new method will be called serverSupports. I'll use twisted.python.deprecate.deprecated in the __init__ to mark isupport as deprecated (this will allow to mark the function even if the user has overridden it) and TestCase.callDeprecated instead of assertWarns in the tests. I won't add here the channelmodes instance attribute (usermodes and userprefixes will be added - see comment 11) because it's not needed anymore in #3296.

comment:16 Changed 11 years ago by Jean-Paul Calderone

decorating an instance method might have bad side-effects. It would probably be less surprising to just emit a deprecation warning when you're going to call it when you notice it is overridden. The problem with this is that there's no easily-testable way to emit a deprecation warning without using twisted.python.deprecate.deprecated. We should really address that point though, rather than come up with somewhat gross work-arounds.

Changed 11 years ago by Ezio Melotti

Attachment: ticket3285.2.patch added

comment:17 Changed 11 years ago by Ezio Melotti

I added the new patch, it now has the tests too and everything works fine. I had some problems whit test_deprecatedISupportMethod and I end up with using assertWarns. As you can see from the commented code there I tried several (hackish) things with no results.

If the patch is ok I'll remove the commented code and submit it for review.

comment:18 Changed 11 years ago by Ezio Melotti

Keywords: review added
Owner: Ezio Melotti deleted

comment:19 Changed 11 years ago by Jean-Paul Calderone

author: exarkun
Branch: branches/isupport-3285

(In [24266]) Branching to 'isupport-3285'

comment:20 Changed 11 years ago by Jean-Paul Calderone

(In [24267]) Apply ticket3285.2.patch

refs #3285

comment:21 Changed 11 years ago by Jean-Paul Calderone

(In [24268]) Fix trivial coding standard violations

refs #3285

comment:22 Changed 11 years ago by Jean-Paul Calderone

author: exarkunWolf
Keywords: review removed
Owner: set to Ezio Melotti
  • irc_RPL_ISUPPORT should dispatch to methods named prefix_SUFFIX - eg, instead of _parseISupportArg, dispatch to _isupport_ARG
  • server_capab should be renamed something like serverCapabilities. Abbreviations should be limited to widely used abbreviations or cases where the shorter name is significantly more readable than the full version; the _ should be omitted in any case, since that's not how attributes are named according to the coding standard.
  • userprefix and usermodes should be documented in the class docstring, not the connectionMade method docstring

The deprecation stuff seems right to me now, but it'll be more clear when the commented out dead code is removed. Please submit a patch against the branch to do that, as well as to fix the above issues. Thanks!

comment:23 in reply to:  22 Changed 11 years ago by Ezio Melotti

Keywords: review added
Owner: Ezio Melotti deleted

Replying to exarkun:

  • irc_RPL_ISUPPORT should dispatch to methods named prefix_SUFFIX - eg, instead of _parseISupportArg, dispatch to _isupport_ARG

I left _parseISupportArg as it is and renamed _parseISupportPrefix to parseISupport_PREFIX. irc_RPL_ISUPPORT will dispatch the parameters that require further parsing to methods named parseISupport_PARAMNAMEHERE.

  • server_capab should be renamed something like serverCapabilities.

Done

  • userprefix and usermodes should be documented in the class docstring, not the connectionMade method docstring

Done, I also added there the doc for serverCapabilities and removed it from the methods' docstrings

I also removed the commented code, replaced all the assertEquals() with assertEqual() in test_irc.py, fixed some docstrings and added a couple of comments.

Changed 11 years ago by Ezio Melotti

Attachment: ticket3285.3.patch added

comment:24 Changed 11 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Ezio Melotti

ticket3285.3.patch doesn't apply cleanly to the branch. What did you generate it from?

Changed 11 years ago by Ezio Melotti

Attachment: ticket3285.4.patch added

comment:25 Changed 11 years ago by Ezio Melotti

Keywords: review added
Owner: Ezio Melotti deleted

This one should work.

Changed 11 years ago by Ezio Melotti

Attachment: ticket3285.5.patch added

Changed 11 years ago by Ezio Melotti

Attachment: ticket3285.6.patch added

comment:26 Changed 11 years ago by Jean-Paul Calderone

(In [24324]) Apply ticket3285.6.patch

refs #3285

comment:27 Changed 11 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Ezio Melotti

Thanks. A bunch of fairly minor things:

  • _checkServerSupports docstring misspells serverSupports
  • _checkPrefixAndServerCapab docstring misspells variable
  • test_serverSupportsWithSingleMessage and test_serverSupportsWithMultipleMessages should explain what correct result means. In general, using correct in a test method docstring is not the best idea: explain what correct means instead.
  • The _delISupportFromClient in test_deprecatedISupportMethod is probably superfluous - what difference will it make if that doesn't happen?
  • The change to testMultipleLine means the two lines following it need to be re-indented.
  • @ivar and other epytext markup should have the second and following lines of multiline text indented 4 spaces - the three new @ivars on IRCClient are missing this
  • The deprecation irc_RPL_BOUNCE emits is good, but it would be best if the message said exactly where the deprecated code was - for example, in this case, it'd be great if it gave the name of the class which has the isupport attribute.
  • irc_RPL_ISUPPORT uses hasattr - use getattr with a default instead: hasattr can swallow unexpected exceptions.
  • irc_RPL_BOUNCE also uses hasattr, and the self.isupport.im_func is not IRCClient.isupport.im_func part of the check for calling isupport is untested
  • _parseISupportArg calls rstrip('='), but it doesn't need to, if there's an =, there won't be a ValueError raised. Also, that exception handler is untested.
  • All of the log messages/early returns in parseISupport_PREFIX are untested and there's a NameError in one of them.
  • The ^ at the beginning of the pattern in parseISupport_PREFIX is unneeded - re.match implies that.

comment:28 Changed 11 years ago by David Sturgis

(In [26279]) re #3285, serverInfo has been marked private with an underscore, and the blank lines have been unremoved to clean up the diff

comment:29 Changed 11 years ago by Jonathan Jacobs

Most ISUPPORT parameters require special parsing, some also have default values. I've attached a patch that introduces an ISUPPORT parser object which follows the guidelines laid out in http://www.irc.org/tech_docs/draft-brocklesby-irc-isupport-03.txt. The patch was constructed against trunk.

According to that same document, RPL_BOUNCE is so scarcely implemented that they've changed it to numeric 010 and left RPL_ISUPPORT as 005.

Changed 11 years ago by Jonathan Jacobs

Attachment: isupport-parser.diff added

comment:30 Changed 11 years ago by Jonathan Jacobs

Keywords: review added

comment:31 Changed 11 years ago by Jonathan Jacobs

Owner: Ezio Melotti deleted

comment:32 Changed 11 years ago by Jonathan Jacobs

I realise there may be a number of changes that were present in previous patches that do not appear in mine; my feeling is that the majority of these changes don't seem like a good way to move forward: raising exceptions during parsing ISUPPORT parameters, introducing IRCClient.serverSupports (in my opinion, this function (along with the original IRCClient.isupport function) add little to no value.)

I'm generally always available on IRC (as k4y) to discuss any of my comments or patches.

comment:33 Changed 11 years ago by Glyph

Keywords: review removed
Owner: set to Jonathan Jacobs

IRC's terribleness boggles my mind. Thank you for attempting this. I will attempt to write a review, but I feel unqualified.

  1. The naming of ISupport is unfortunate. It looks like an interface. I'd suggest naming it something a bit more verbose, like ServerSupportedFeatures.
  2. I don't like the dict-like-object design. If an object has a set of features which each have distinct semantics, then it seems to me that there should be a set of methods or attributes with documentation that explain their function. This is useful when reasoning about code, and also useful for automated tools; there's a hope that e.g. Eclipse can figure out what type someServerSupportedFeaturesInstance.channelModes is, but it's pretty close to zero that it will figure out what maybeThisIsADictIDunno["CHANMODES"] is.
  3. CommandDispatcherMixin has a problem with inheritance; you can't mix it in twice for dispatching different prefixes. That's fine as a limitation, but (A) perhaps it shouldn't be public, and (B) the limitation should be documented.
    1. I don't see that _dispatch adds any value; why have two methods where one will suffice?
  4. _intOrDefault needs a docstring.
  5. There's an XXX in isupport_CHANMODES. I don't think I understand the protocol involved here well enough to recommendation one way or another, but implement something rather than leave an XXX there :).
  6. IRCClient.isupported needs an @ivar. Also consider a name parallel with ISupport's new class name (serverSupportedFeatures?)
  7. test_irc.Dispatcher and its methods need docstrings.

As far as I can tell, this is otherwise in good shape...

Changed 10 years ago by Jonathan Jacobs

Attachment: isupport-parser-1.diff added

comment:34 Changed 10 years ago by Jonathan Jacobs

Keywords: review added
Owner: changed from Jonathan Jacobs to Glyph
  1. I've removed the dict-like-object design and implemented getFeature and hasFeature instead, attributes are tricky when you start changing CHANMODES to channelModes. I think it's better to let everyone address the features using their original names.
  1. 1. I don't have any use case right now to back this up, so I've reduced it to a single function.

I've addressed all the other points too.

comment:35 Changed 10 years ago by Glyph

Owner: Glyph deleted

comment:36 Changed 10 years ago by Jean-Paul Calderone

Author: Wolfjonathanj
Keywords: review removed
Owner: set to Jonathan Jacobs
  1. Some of _intOrDefault is not covered by any tests
  2. The first check in ServerSupportedFeatures._parsePrefixParam is not covered by any tests
  3. The last line of ServerSupportedFeatures._parsePrefixParam seems overly complex. How about dict(zip(modes, symbols))?
  4. ServerSupportedFeatures.isupport_CHANMODES is entirely untested
  5. ServerSupportedFeatures.isupport_IDCHAN is entirely untested
  6. ServerSupportedFeatures.isupport_MAXLIST is entirely untested
  7. ServerSupportedFeatures.isupport_NETWORK is entirely untested
  8. ServerSupportedFeatures.isupport_SAFELIST is entirely untested
  9. ServerSupportedFeatures.isupport_STATUSMSG is entirely untested
  10. ServerSupportedFeatures.isupport_TARGMAX is entirely untested
  11. hasFeature isn't really any different from __contains__ (or whatever the dict-like API glyph complained about was). A good high-level API would indeed things like channelModes and such. However, a good low-level API doesn't need to do this. So perhaps this is only the low-level API. I don't think this point needs to block this ticket.

Changed 10 years ago by Jonathan Jacobs

Attachment: isupport-parser-2.diff added

comment:37 Changed 10 years ago by Jonathan Jacobs

Keywords: review added
Owner: changed from Jonathan Jacobs to Jean-Paul Calderone

I've addressed points 1 to 10.

Regarding point 11: My only concern about introducing attributes like channelModes is that there's no real way to figure out that what is specified as CHANMODES in the RFC is now named channelModes (this particular example is relatively obvious, but there are some more esoteric names in the RFC), apart from trawling through the documentation or source.

Things get trickier when you consider that every IRC server implementation introduces their own ISUPPORT parameters, so maybe SPAMEGGS is supported but there's no spamEggs attribute, so now do you have to fish around in a dictionary of unknowns? What happens when Twisted does support SPAMEGGS? Does the SPAMEGGS value in the dictionary of unknowns go away suddenly?

ISUPPORT is low-level, anything higher level should almost certainly be handled (or exposed?) by IRCClient.

comment:38 Changed 10 years ago by Jean-Paul Calderone

Owner: Jean-Paul Calderone deleted

Changed 10 years ago by Jonathan Jacobs

Attachment: isupport-parser-3.diff added

comment:39 Changed 10 years ago by Jean-Paul Calderone

Author: jonathanjjonathanj, exarkun
Branch: branches/isupport-3285branches/isupport-3285-2

(In [27136]) Branching to 'isupport-3285-2'

comment:40 Changed 10 years ago by Jean-Paul Calderone

(In [27137]) Apply isupport-parser-3.diff

refs #3285

comment:41 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jonathan Jacobs
  1. The test docstrings are written in a somewhat strange style. They're generally missing a subject and in a funny tense. Preferred style is along the lines of foo does bar.
  2. assertEquals is preferred over assertEqual now.
  3. Generally we don't vertically align things (as the patch does in, eg, test_support_CHANMODES)
  4. IDCHANs seems to be a list, conceptually, not a tuple. Also, the test for this has a docstring which doesn't really go into sufficient detail. In general, if you're describing something as "correct" in a test docstring, you're not going into enough detail.
  5. test_support_MAXLIST docstring talks about a mapping, but the test does things with tuples.
  6. test_support_NETWORK docstring isn't detailed enough. Ditto for test_support_SAFELIST, test_support_STATUSMSG.
  7. isupport_NICKLEN is implemented with an _intOrDefault call which passes in a default value, but no test exercises this default value feature.

comment:42 Changed 10 years ago by Jean-Paul Calderone

I started off addressing these review points because I thought it'd be minimal and then I could merge the branch, but it didn't end up that way. I mostly addressed point 1 already, and think I covered 2, 3, and 4 (all in r27138).

Changed 10 years ago by Jonathan Jacobs

Attachment: isupport-parser-4.diff added

Created against isupport-3285-2 branch

comment:43 Changed 10 years ago by Jonathan Jacobs

Keywords: review added
Owner: changed from Jonathan Jacobs to Jean-Paul Calderone
  1. I think that what you said about IDCHANS makes sense for just about every use of _splitParameterArgs, so I changed it return a list and modified the relevant code.
  2. Some isupport_ methods were reported not to be covered by tests, so I added tests for these (even though they're pretty trivial) and isupport_NICKLEN falls into this category.

I improved the docstrings for the methods in points 5 and 6.

As I mentioned on IRC, the "PREFIX" support parameter's arguments appear in a significant order (which the dict implementation loses.) I followed your suggestion to introduce a priority value and updated the tests to both know about this value and to make sure it is correct.

comment:44 Changed 10 years ago by Jean-Paul Calderone

(In [27139]) Apply isupport-parser-4.diff

refs #3285

comment:45 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Jonathan Jacobs
  1. As discussed on IRC, let's make isupported get called after the supported thingy is made consistent with the message being dispatched.
  2. I don't think there is test coverage for the default features/feature values
  3. I don't think there is test coverage for the or 'e' in isupport_EXCEPTS

Changed 10 years ago by Jonathan Jacobs

Attachment: isupport-parser-5.diff added

Created against isupport-parser-4.diff applied to isupport-3285-2 branch.

comment:46 Changed 10 years ago by Jonathan Jacobs

Keywords: review added
Owner: changed from Jonathan Jacobs to Jean-Paul Calderone

I addressed all of your queries. Implementing some of these tests exposed some bugs, which I (hopefully) patched.

comment:47 Changed 10 years ago by Jean-Paul Calderone

(In [27223]) Apply isupport-parser-5.diff

refs #3285

comment:48 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Jonathan Jacobs

Some lingering questions (some of which we might have discussed on IRC a while ago), and other stuff:

  1. Does ServerSupportedFeatures need to be part of the public API (specifically, the ability for application code to instantiate them, subclass it, etc)?
  2. Does the parse method need to allow multiple ISUPPORT strings to be parsed into the same ServerSupportedFeatures instance?
  3. isupport_INVEX is incompletely tested (its single line is only executed once by the whole test suite, and there are at least two paths through it...)

comment:49 Changed 10 years ago by Jonathan Jacobs

Keywords: review added
Owner: changed from Jonathan Jacobs to Jean-Paul Calderone
  1. It probably doesn't, but as you mentioned on IRC, making it private does hamper documentation somewhat. I think maybe leaving it public might be the better option.
  1. As discussed, I've added some comments / documentation indicating why we need to be able to call parse multiple times to mutate ServerSupportedFeatures.
  1. Added a test for this.

comment:50 Changed 10 years ago by Jonathan Jacobs

(For clarity, I committed to the branch, instead of attaching another patch.)

comment:51 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Jonathan Jacobs

Noticed one more thing. Previously, IRCClient had no __init__. This means that a subclass which overrode __init__ could not invoke the base implementation. However, with this change, subclasses must invoke the base implementation, or they'll get an AttributeError raised as soon as ISUPPORT is received. This would be an unfortunate incompatibility. Please add a test for this scenario and make it pass (should be easy - connectionMade is a good place for protocol initialization). Perhaps it would be good for NoticingClient to go back to something like its previous behavior, with a note about how it's important that it doesn't call IRCClient.__init__ for test coverage purposes.

Hopefully this is the last bit of feedback I have.

comment:52 Changed 10 years ago by inhahe

Priority: highnormal

I noticed that on a certain dalnet server the patch would raise an exception because TARGMAX passed int as the valueProcessor, and the value the server passed wasn't an int. so i changed all places where it passes int to _intOrDefault.

but i don't like the fact that if the server passes something other than an int, its value is just stored as None.. what if the isupport implementation doesn't recognize the format but an irc client wants to access the value anyway? i figured teh easiest solution (easiest to implement and backward-compatible) to this would be to create a second dictionary which stores key/values for things that were stored as None so in parse after

self._features[key] = self.dispatch(key, value)

i put

if self._features[key] is None:

self._features2[key] = value

i'm not sure if that's right, if value there is what i want, and i know _features2 isn't a good name, but just to show you what my idea is. since the features dicts are internal (they have an underscore) i guess i would ideally also provide a separate getFeature function for _features2 so the client would do

if self.getFeature('TARGMAX') is None:

#do something with getFeature2('TARGMAX')

i figured that's more elegant than just doing

if self._features[key] is None:

self._features[key] = value

because then the irc client doesnt have to do

if not isinstance(getFeature('TARGMAX'), int):

...

anyway just an idea, there could be a better way

comment:53 Changed 10 years ago by inhahe

Priority: normalhigh

[when i say 'i change' i mean in my own personal copy of irc.py. i don't do commits and such.]

comment:54 Changed 10 years ago by Jean-Paul Calderone

What does dalnet supply for TARGMAX? And why is it not a protocol error for it to supply something that's not an integer?

comment:55 Changed 10 years ago by inhahe

I'm not sure it's all of dalnet. I think it was just some particular server. While I'm not an IRC expert, my impression is that a lot of "standards" in the irc protocol are only standards by convention and IRC servers are highly prone to individually expand and defy standards and conventions.

Especially with isupport, i would think, where the entire list of isupport keys falls under one IRC command which was added later, and some key values are expected to be ints, some strings, and some, perhaps, either ints or strings, and the keys you might receive are arbitrary enough that irc.py's isupport was programmed to process and remember any key/value pair the server sends whether the key is recognized as standard or not.

so basically i wouldnt consider it a 'protocol error' in such a way that i'd justify throwing an exception over it. it seems the module should be more flexible than that. also, as long as you have _intOrDefault, it seems to just make sense to at least use it instead of int, it even makes me wonder if not doing so was an oversight.

comment:56 Changed 10 years ago by inhahe

here's something else i just added to my own personal copy of irc.py. an irclower() function. feel free to take my code and convert it to reasonable programming practices.

[somewhere near the top..]

irclowertranslations = {

"ascii": string.maketrans(string.uppercase, string.lowercase), "rfc1549": string.maketrans(string.uppercase + "\x7B\x7C\x7D\x7E",

string.lowercase + "\x5B\x5C\x5E\x5F"),

"strict-rfc1549": string.maketrans(string.uppercase + "\x7B\x7C\x7D",

string.lowercase + "\x5B\x5C\x5E")

}

[somewhere in the IRCClient class..]

def irclower(self, text):

try:

trans = irclowertranslations[self.supported.getFeature("CASEMAPPING")]

except:

trans = irclowertranslationsrcf1549?

return text.translate(trans)

information gleaned from

http://www.irc.org/tech_docs/draft-brocklesby-irc-isupport-03.txt

and

http://code.google.com/p/zbnc/issues/detail?id=1

the code is untested, so if it doesn't work hopefully the basic idea is clear enough..:)

comment:57 Changed 10 years ago by Jonathan Jacobs

(In [27436]) Make ISUPPORT integer parameter argument parsing more robust. refs #3285

comment:58 Changed 10 years ago by Jonathan Jacobs

(In [27438]) Move important stuff in IRCClient from init to connectionMade. refs #3285

comment:59 Changed 10 years ago by Jonathan Jacobs

Replying to inhahe:

I noticed that on a certain dalnet server the patch would raise an exception because TARGMAX passed int as the valueProcessor, and the value the server passed wasn't an int. so i changed all places where it passes int to _intOrDefault.

This is probably a good idea.

but i don't like the fact that if the server passes something other than an int, its value is just stored as None.. what if the isupport implementation doesn't recognize the format but an irc client wants to access the value anyway? i figured teh easiest solution (easiest to implement and backward-compatible) to this would be to create a second dictionary which stores key/values for things that were stored as None

This is a potential problem, however it is also an extreme edge case. The chances of the value of TARGMAX not being an integer but still being useful must be pretty close to zero. If this is a real and pressing issue, I would suggest filing another ticket for introducing a new API to look up values that could not be parsed.

Replying to inhahe:

here's something else i just added to my own personal copy of irc.py. an irclower() function.

I don't think that the old RFC1459 casemapping is an issue worth considering. Even if it is, it probably belongs in a new ticket.

comment:60 in reply to:  51 Changed 10 years ago by Jonathan Jacobs

Keywords: review added
Owner: changed from Jonathan Jacobs to Jean-Paul Calderone

Replying to exarkun:

Noticed one more thing. Previously, IRCClient had no __init__. This means that a subclass which overrode __init__ could not invoke the base implementation. However, with this change, subclasses must invoke the base implementation, or they'll get an AttributeError raised as soon as ISUPPORT is received. This would be an unfortunate incompatibility. Please add a test for this scenario and make it pass (should be easy - connectionMade is a good place for protocol initialization). Perhaps it would be good for NoticingClient to go back to something like its previous behavior, with a note about how it's important that it doesn't call IRCClient.__init__ for test coverage purposes.

Yes, this is a good point. One that would probably have broken every existing use of IRCClient. Nice catch.

comment:61 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Jonathan Jacobs

There's a conflict in the IRCClient class docstring. It looks like a simple one to resolve - keep both pieces - so I'll do the review anyway. Please merge forward and resolve the conflict before merging to trunk. Also, you'll have to merge forward to get a clean test run on buildbot anyway, as this old branch includes unrelated failing tests.

Aside from that, I think you just need to add tests for the _intOrDefault uses in isupport_MAXLIST and isupport_TARGMAX.

comment:62 Changed 10 years ago by Jonathan Jacobs

Branch: branches/isupport-3285-2branches/isupport-3285-3

(In [27459]) Branching to 'isupport-3285-3'

comment:63 Changed 10 years ago by Jonathan Jacobs

(In [27460]) Merge forward. refs #3285

comment:64 Changed 10 years ago by Jonathan Jacobs

(In [27461]) Add tests for the _intOrDefault uses in isupport_MAXLIST and isupport_TARGMAX. refs #3285

comment:65 Changed 10 years ago by Jonathan Jacobs

Resolution: fixed
Status: newclosed

(In [27463]) Merge isupport-3285-3.

Author: jonathanj Reviewer: exarkun, inhahe Fixes: #3285

Facilitate the parsing and handling of ISUPPORT messages, in IRCClient, with a new object while still maintaining compatability with the legacy ISUPPORT API.

Note: See TracTickets for help on using tickets.