Opened 6 years ago

Closed 6 years ago

#3230 defect closed fixed (fixed)

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

Reported by: Wolf Owned by:
Priority: normal Milestone:
Component: words Keywords: irc, IRCClient, irc_MODE, modeChanged, review
Cc: Branch:
Author: Wolf Launchpad Bug:

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 Wolf 6 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 in reply to: ↑ description Changed 6 years ago by Wolf

  • 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 6 years ago by 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)

comment:3 follow-up: Changed 6 years ago by therve

  • Keywords review removed
  • Owner changed from exarkun to Wolf

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 6 years ago by Wolf

  • 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 6 years ago by Wolf

  • Owner Wolf deleted

comment:6 follow-up: Changed 6 years ago by therve

  • Keywords review removed
  • Owner set to Wolf
  • 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 6 years ago by Wolf

  • Keywords review added
  • Owner Wolf 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 6 years ago by Wolf

comment:8 Changed 6 years ago by trac

  • Milestone Words 8.0.0 + 1 deleted

Milestone Words 8.0.0 + 1 deleted

comment:9 Changed 6 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by <automation>

Note: See TracTickets for help on using tickets.