Opened 10 years ago

Closed 10 years ago

Last modified 14 months ago

#3230 defect closed fixed (fixed)

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

Reported by: Ezio Melotti Owned by:
Priority: normal Milestone:
Component: words Keywords: irc, IRCClient, irc_MODE, modeChanged
Cc: Branch:
Author: Wolf

Description

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.

Attachments (1)

ticket3230.patch (10.7 KB) - added by Ezio Melotti 10 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 in reply to:  description Changed 10 years ago by Ezio Melotti

Keywords: irc_MODE review added

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

comment:2 Changed 10 years ago by Ezio Melotti

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)

comment:3 Changed 10 years ago by therve

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Ezio Melotti

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.

comment:4 in reply to:  3 Changed 10 years ago by Ezio Melotti

Keywords: review added

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

comment:5 Changed 10 years ago by Ezio Melotti

Owner: Ezio Melotti deleted

comment:6 Changed 10 years ago by therve

Keywords: review removed
Owner: set to Ezio Melotti
  • 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.

comment:7 in reply to:  6 Changed 10 years ago by Ezio Melotti

Keywords: review added
Owner: Ezio Melotti deleted

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.

Changed 10 years ago by Ezio Melotti

Attachment: ticket3230.patch added

comment:8 Changed 10 years ago by trac

Milestone: Words 8.0.0 + 1

Milestone Words 8.0.0 + 1 deleted

comment:9 Changed 10 years ago by therve

Resolution: fixed
Status: newclosed

(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

comment:10 Changed 7 years ago by <automation>

comment:11 Changed 14 months ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.