Opened 4 years ago

Last modified 18 months ago

#4929 enhancement new

Add LIST to IRCClient

Reported by: taos Owned by: Upasana
Priority: normal Milestone:
Component: words Keywords:
Cc: ralphm, me@… Branch:
Author: Launchpad Bug:

Description

Simply making a method that calls '/LIST' This is done in IRCClient by adding a channel_list method (knowing that list is a keyword.).

Attachments (6)

4929.patch (1.5 KB) - added by taos 4 years ago.
Patch containing method and unit tests.
bug_4929.patch (3.3 KB) - added by Upasana 19 months ago.
bug_6482_2.patch (335 bytes) - added by Upasana 19 months ago.
ircLogBot.py (5.1 KB) - added by Upasana 19 months ago.
Use Case for my patch
README (749 bytes) - added by Upasana 19 months ago.
This is a README file for ircLogBot.py
bug_4929_2.patch (4.2 KB) - added by Upasana 19 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 4 years ago by DefaultCC Plugin

  • Cc ralphm added

Changed 4 years ago by taos

Patch containing method and unit tests.

comment:2 Changed 4 years ago by washort

  • Keywords review removed
  • Owner set to taos

This patch doesn't implement irc_RPL_LIST so sending the LIST command will have no effect at all for your client.

comment:3 Changed 19 months ago by Upasana

  • Cc me@… added
  • Owner changed from taos to Upasana

Changed 19 months ago by Upasana

comment:4 Changed 19 months ago by Upasana

  • Cc me@… removed
  • Keywords review added
  • Owner Upasana deleted

comment:5 Changed 19 months ago by Upasana

  • Cc me@… added

comment:6 Changed 19 months ago by acapnotic

  • Owner set to acapnotic
  • Status changed from new to assigned

assigning for review

comment:7 follow-up: Changed 19 months ago by acapnotic

  • Keywords review removed
  • Owner changed from acapnotic to Upasana
  • Status changed from assigned to new

Looking at this in parts. First, channel_list:

That looks like a fair test. If you had many more examples to test, you'd likely want to break it up into multiple assertions, but I think this is fine as it is.

Stylistically, the method name should be "channelList", and this case "LIST %s" % (channel) should be "LIST %s" % (channel,), with the comma to ensure that it's always a tuple that you're passing as the format args.

The RFC suggests that one may pass multiple channels to list, and some quick experimentation shows that some servers do accept that, so we should support that interface.

As for the reply methods, I am less clear. Do you have an example use case that demonstrates what a user of this module would do with them?

I am guessing gotChannelList and channelListEnd would be more useful with more meaningful arguments, i.e. gotChannelList(channel, userCount, topic). With numbers passed as ints, not strings.

Why do those methods have return values?

Finally, the IRCClient class too many methods defined on it. This is not a problem we are going to solve today! But I think it does make sense to stick to the attempt of organization it has, putting your new methods under the sections defined by the comments that are there:

### Interface level client->user output methods

gotChannelList
channelListEnd

### user input commands, client->server

channelList

### server->client messages

irc_RPL_LIST
irc_RPL_LISTEND

Some of those sections may have sub-sections of their own, check to see where these fit best.

Thanks!

comment:8 in reply to: ↑ 7 Changed 19 months ago by Upasana

Replying to acapnotic:

Looking at this in parts. First, channel_list:

That looks like a fair test. If you had many more examples to test, you'd likely want to break it up into multiple assertions, but I think this is fine as it is.

Stylistically, the method name should be "channelList", and this case "LIST %s" % (channel) should be "LIST %s" % (channel,), with the comma to ensure that it's always a tuple that you're passing as the format args.

The RFC suggests that one may pass multiple channels to list, and some quick experimentation shows that some servers do accept that, so we should support that interface.

As for the reply methods, I am less clear. Do you have an example use case that demonstrates what a user of this module would do with them?

I am guessing gotChannelList and channelListEnd would be more useful with more meaningful arguments, i.e. gotChannelList(channel, userCount, topic). With numbers passed as ints, not strings.

Why do those methods have return values?

Finally, the IRCClient class too many methods defined on it. This is not a problem we are going to solve today! But I think it does make sense to stick to the attempt of organization it has, putting your new methods under the sections defined by the comments that are there:

### Interface level client->user output methods

gotChannelList
channelListEnd

### user input commands, client->server

channelList

### server->client messages

irc_RPL_LIST
irc_RPL_LISTEND

Some of those sections may have sub-sections of their own, check to see where these fit best.

Thanks!

Thanks for the review.

comment:9 Changed 19 months ago by acapnotic

Oh, and an addition such as this will also need a NEWS entry as described at http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

comment:10 Changed 19 months ago by Upasana

Alright, will read it before submitting my next patch. Thanks.

Changed 19 months ago by Upasana

Changed 19 months ago by Upasana

Use Case for my patch

Changed 19 months ago by Upasana

This is a README file for ircLogBot.py

comment:11 Changed 19 months ago by Upasana

  • Keywords review added
  • Owner Upasana deleted

Changed 19 months ago by Upasana

comment:12 Changed 18 months ago by tom.prince

  • Owner set to Upasana

Thanks for your contribution. A couple of minor issues:

  1. Regrading supporint passing multiple channels, it should take a list of channels, rather than a string. The purpose is to abstract away the details of IRC, so the user shouldn't need to know how to format the list.
  2. There aren't any tests for receiving replies. There should be some code that calls irc_RPL_LIST*, and then asserts that the correct 'client->user' methods get called.
  3. The 'client->user' commands should probably be in another subsection. Perhaps 'Methods relating to the server itself' or 'Information from the server.' or new one.

Please resubmit for review after addressing the above points.

  1. Looking through the existing code, I noticed that motd has the same pattern (RPL_*START, RPL_*, RPL_*END), but that that code collects all the responses, and presents them to the user all at once.
    • I'm not sure if one approach is better than the other. The motd requires less work from the user, but doesn't behave well if the server behaves improperly, and doesn't allow processing results incrementally (which would be important for handling all the channels on freenode, for example).

comment:13 Changed 18 months ago by tom.prince

  • Keywords review removed
Note: See TracTickets for help on using tickets.