IRCClient.modeChanged fails to handle different modes at the same time

[Wolf]

This is the testcode:

class MyIRCClient(irc.IRCClient):
    def modeChanged(self, user, channel, set, modes, args):
        print 'Mode changed:', user, channel, set, modes, args

When I set a single mode (e.g. +c) or I add/remove several modes (e.g. +cU) it works (it prints 'Mode changed: user channel True cU ()').

When I set different modes (e.g. +c-U) it prints 'Mode changed: user channel True c-U ()'.

In this situation the expected result should be something like (True, False) ('c', 'U') or a list like [(True, 'c'), (False, 'U')], or better a plain string '+c-U' (the one that the server already sends, unchanged).

These solutions will probably break the compatibility with the previous version (a tuple instead of a bool in the 1st case, a single parameter instead of 'set' and 'modes' with a list or a string for the other two). Another possible solution is to call two (or more) times the modeChanged method (i.e. the 1st with set=True, modes='c' and the 2nd with set=False, modes='U'). This won't allow the user to handle the modes together (XChat uses a similar solution too, mIRC instead handles all the changes in a single message) but it shouldn't break the compatibility with the previous version of twisted.

If the compatibility is not a problem, sending the string is probably (IMHO) the best solution, it will save you some work and it will allow us to parse it as we want.

The documentation should also be changed (see also ticket #3148).

The modeChanged method of the IRCClient class is at the line 701 of the irc.py module in twisted/words/protocols.

[Wolf]

Now irc_MODE() can handle different modes at the same time (e.g. +c-U or +c-U+ns-rt), calling modeChanged() twice (the 1st with set=True, modes='cns' and the 2nd with set=False, modes='Urt').

I also improved the doc of modeChanged() (see ticket #3148) and added 7 new tests in test_irc.py

[Wolf]

Added 3 more tests and a dict used to check if a mode accepts an arg or not (e.g. -c+o nick -> "nick" has to go with +o and not with -c)

[therve]

Thanks a lot for your contribution. I don't think there are major problems, just small cleanups:

• tests should be named 'test_modeXXX', not 'test_ModeXXX'
• tests methods need docstrings explaining the goal of the test: expected behavior, etc
• there is a lot of duplication in the tests: you should create a test helper, and just pass the set/modes/args arguments to it
• there are several missing spaces after commas in irc_MODE

Thanks again.

[Wolf]

Added 3 helper methods for the tests, refactored the testModeChange() method (now test_modeChangeWithASigleMode) to use them, added the docstrings and the missing spaces.

[therve]

• It would be useful to know where you got the modes list, and to explain the modes present by a small comment for each. I think the modes that doesn't take arguments should not be in there: just rely on the (False, False) in the get
• docstring are formatted this way:

  """
  A docstring.
  """
  

• there should be 2 blank lines between methods
• I think some tests should be added to check what happens when parsing fails (when the server sends wrong data). You don't have to change the behavior of irc_MODE, but just to check what happens

Thanks.

[Wolf]

Added the url of the RFC, a small comment for every mode, removed common modes with (False,False) not included in the RFC (I left the h (halfop) that afaik is quite common). Fixed the docstring and added 2 blank lines between the methods. Improved irc_MODE to handle some errors (MODE s -> MODE +s; MODE +a+b+c-x-y-z -> MODE +abc-xyz) and added 3-4 more tests to check if it does it correctly. If after the parsing there are some unassigned arg a message will now be written in the log to warn the user.

[trac]

Milestone Words 8.0.0 + 1 deleted

[therve]

(In [23671]) Fix IRCClient so that's it's able to handle modes set and unset at the same time; to keep compatibility with IRCClient.modeChanged, it's called 2 times, the first with modes set and the second with modes unset.

Author: Wolf Reviewer: therve Fixes #3230