Improvements and fixes

[mrdumpty]

Hello.

I 've made some improvements and refactored your code a little bit (made more pythonic i.e. "If var is None" -> just if var, changed deprecated api calls, etc)

Please review it.

[shawnanastasio]

Hi, thanks for the contribution.

Overall, it looks good to me, with the exception of the send_message function you added. Could you explain your rationale for adding it? Also, it seems to me that the ability to send a message to all rooms doesn't have much use outside of spamming.

Also, I saw on PR #13 that you mentioned that this PR implements the same functionality, but as far as I can tell it still only allows room objects to be passed in to the constructor. Am I missing something here?

[mrdumpty]

Hi, For the "send_message" the main idea is that sending messages obviously is the one of main purposes of any chat bot, is not it? As developer, I feel much more comfortably with simple interface for it. But I can remove it, if it is not how you see the basic concept. Sending to all rooms is useful for some types of broadcasting messages, i.e. emergency reports (we are using it in our monitoring system).

What about this PR I dont think that exclusive flag is good idea, just specifying the rooms is enough.

but as far as I can tell it still only allows room objects to be passed in to the constructor.

In your code, room is a text ID in one place, and object in another. On line 35 if rooms is None, your code is appending room_id to "rooms" list, but else it assumes that "rooms" list contains room object and adding listeners for them. I just removed ambiguity and "rooms" contains room objects anyway, but it may be more useful if api accepted IDs or aliases, may be?

[shawnanastasio]

For the "send_message" the main idea is that sending messages obviously is the one of main purposes of any chat bot, is not it?

Of course. The current way this is meant to be done is by calling the send_text method on the room object passed to the callback. Indeed, the entire library is centered around this callback-driven model, and to me, the addition of another method to accomplish this that uses global state only complicates things.

If users wish to send messages to a room without being triggered by a handler, it seems like the better solution is to pick out a room from the bot's rooms list and call the send_text method on that.

Sending to all rooms is useful for some types of broadcasting messages, i.e. emergency reports (we are using it in our monitoring system)

I hadn't considered this. If you agree with my previous comment, I think it would be good to replace your send_message method with a broadcast_message method that simply broadcasts a given message to all joined rooms. If users need more granularity (i.e. broadcasting to only certain rooms), accessing the bot's rooms list as I described earlier should be done instead.

In your code, room is a text ID in one place, and object in another. On line 35 if rooms is None, your code is appending room_id to "rooms" list, but else it assumes that "rooms" list contains room object and adding listeners for them.

Good catch. Could you explain the rationale for the print statement you added though? It doesn't seem particularly useful to me other than for library debugging purposes.

but it may be more useful if api accepted IDs or aliases, may be?

This, I believe, is what PR #13 implements. I do agree about the exclusive flag though, I'll ask for clarification on that from the author.