Add channel plugin for state of channels and users

[nitori]

Keeps track of currently joined channel and it's users.

[FichteFoll]

Here is how I would imagine the channel API to work, with the example of reimplementing your two commands from test.py:

    n_ctx: NetworkContext

    def dehighlight(nick):
        return f"{nick[:1]}\N{ZERO WIDTH SPACE}{nick[1:]}"

    @n_ctx.channel_event(ChannelEventName.MESSAGE)
    def on_chanmsg(c_ctx: ChannelContext, message: Message):
        line = message.params[-1]
        words = line.split()

        if words[0] == '!nicks':
            c_ctx.say(' '.join(dehighlight(u.name) for u in c_ctx.users))

        elif words[0] == '!channels':
            c_ctx.say(', '.join(f"{c.name} ({len(c.users)})" for c in c_ctx.network.channels))

I'm a little undecided on the Message class part though. I don't know if we should pre-process all of the possible channel-specific events in order to provide a nicer API (i.e. easier access to sender and line). Events would include stuff like Join, Part, Quit?, Mode, Kick, Topic ... and be namedtuples.

Also, RFC 2811 uses the term "members" instead of "users" (which most irc clients use, I think).

[nitori]

Also, RFC 2811 uses the term "members" instead of "users" (which most irc clients use, I think).

The term "members" kinda means "members of a channel", no? But the users dict doesn't reflect that. The users dict contains "all visible members of all channels without duplicates". If anything you could probably convince me to rename "joins" to "memberships" or something like that, and later have a "get_members" API, or similar, on the channel context, once it's there.

Once again... I really dislike the function nesting.

    @n_ctx.channel_event(ChannelEventName.MESSAGE)
    def on_chanmsg(c_ctx: ChannelContext, message: Message):
        …

This kinda implies it's called from within a function, right? Because n_ctx wouldn't be available. I understand decorators are great 'n stuff, but I really dislike it, when that kind of semi-required nesting is part of the API for registering event callbacks (which is why I "misused" the decorator in this plugin to register top-level functions). And making the network context available from within on_chanmsg is just a matter of passing it to the channel context object.

Don't they say "Flat is better than nested"? :smile_cat:

[FichteFoll]

The term "members" kinda means "members of a channel", no? But the users dict doesn't reflect that.

I know it doesn't do that currently.

But realistically, you don't care about particular users, or if you do, you don't care about which channels they are in because you'd communicate with them directly via PRIVMSG. That means we have the semantic "Server <-1--n-> User" and "Server <-1--n-> Channel <-1--n-> User" relationships, even though technically Channel and User are many-to-many. The point is, however, that we only truly know which channel has which users and not which users is on which channels, because we only know of a limited subset of available channels, so a one-user-to-many-channels relationship would only be half true.

---

This kinda implies it's called from within a function, right? Because n_ctx wouldn't be available.

Not sure I follow with the "function" part.

n_ctx is carried over from the init_network_context, which is basically an "init this plugin" routine, as a clojure. If we were using an inheritation-based approach instead of a functional one, you could think of this as the class's __init__ with each variable definition equating to an attribute assignment to self.

In other frameworks you would probably do this initialization on a module-global level, but because we have multi-network functionality as a goal, with different plugins and plugin settings for each, we can't have plugins share network-specific state globally. As such, a NetworkContext object will always be available in any event handlers, because it is required to register any events on it in the first place. The ChannelContext would just be a layer on top of the network context for events that are specific for a particular channel.

---

I hope I'm making sense here.

[nitori]

This is what I meant. Taken from test.py

@global_event(GlobalEventName.INIT_NETWORK_CTX)
async def init_context(ctx):
    @ctx.message_event('PRIVMSG')
    async def on_privmsg(ctx, message):
        line = message.params[-1]
        words = line.split()
        […]

And expanding it on your example, wouldn't it look something like this?

@global_event(GlobalEventName.INIT_NETWORK_CTX)
async def init_context(n_ctx: NetworkContext):
    @n_ctx.channel_event(ChannelEventName.MESSAGE)
    def on_chanmsg(c_ctx: ChannelContext, message: Message):
        line = message.params[-1]
        words = line.split()

In the channel plugin I instead used something like this (using above example):

def on_chanmsg(c_ctx: ChannelContext, message: Message):
    line = message.params[-1]
    words = line.split()
    # and if network context is required, I'd suggest just accessing it via
    # c_ctx, e.g. c_ctx.network_context or the like.

@global_event(GlobalEventName.INIT_NETWORK_CTX)
async def init_context(n_ctx: NetworkContext):
    n_ctx.channel_event(ChannelEventName.MESSAGE)(on_chanmsg)
    # "misused" the decorator to register on_chanmsg module level function instead of closure.

In other frameworks you would probably do this initialization on a module-global level, but because we have multi-network functionality as a goal, with different plugins and plugin settings for each, we can't have plugins share network-specific state globally.

It would certainly be possible to use a decorator on module level functions as well. I assume init_context would only be called, if the plugin is enabled for said network. So using a module level decorator could store the module level functions in a (global) cache for all the functions that desire to be registered to certain events as if they where registered in init_context, except it's handled by the same routine that dispatches init_context, e.g. pulls out those "desiring" functions, registers them with the specific events (shortly before or after the init_context dispatch, and all with the same "don't execute for this network" logic - if necessary - behind it. The NetworkContext would still be passed normally to each registered function, and we would not have any global network-specific state. Only a global "cache" of functions that desire to be registered.

Well, that's not necessary though. Registering everything in init_context is fine, though I would prefer an additional non-decorator API for that as well. - Simply stating that it's absolutely possible to do. That way we would (if we wanted to do it), completely get rid of init_context in "simple plugins", unless some more advanced things need to be done.

[FichteFoll]

And expanding it on your example, wouldn't it look something like this?

Yes, I just skipped that part and indeted it because I though it was a given.

Your method of specifying the methods globally and just registering them in the init_context is fine and perfectly valid. It loses the advantage of encapsulating network-specific variables through clojures though, which is why you resorted to setting the attributes on ctx directly (which I dislike). In any case, "private" variables that are not supposed to be read by other plugins should be prefixed with an underscore.

I used it myself in the ctcp.py plugin. Though I probably would only use it for ctx.add_method, because imo it's better if the event handler and the event they are listening to are more closely together in code (a decorator would be directly above the code).

and if network context is required, I'd suggest just accessing it via c_ctx, e.g. c_ctx.network_context or the like.

I thought about this, but am undecided. We might as well just do it and see how things go.

So using a module level decorator could store the module level functions in a (global) cache for all the functions that desire to be registered to certain events as if they where registered in init_context

Yes, that is possible as well. It probably makes for a "cleaner API" at first sight, but has the downside of being less flexible and being more troublesome to work with since more magic is involved in the background. It would ease un- and reloading plugins, because we know which events they are listening to, but in order to allow dynamic event hooks we'd have to do that for the current architecture somehow anyway.

Your main critique is that event registration currently is too nested, correct? (I like init_context because it specifically says "this is where the plugin initialization starts". We should probably define a shorthand for the long decorator though. I don't think it is gonna stay this way anyway since it's not network-aware yet, in terms of plugins only being "init"ed for some.)

[nitori]

So, the latest changes basically implement new events for channel/private messages/notices and the channel context. In test.py plugin you can see how I used it.

I used non-caps event names for this (e.g. ChannelMessage), to indicate them being more abstract and not part of the protocol. Though I don't mind them changing.

[FichteFoll]

Things are getting a bit messy now. I'm not so sure which comments of the first review still need addressing, but it wouldn't be hard to spot them again.

Either way, this looks much better now. 👍

Would you mind commenting on my other question earlier? I'm just curious.

Your main critique is that event registration currently is too nested, correct?

[nitori]

Your main critique is that event registration currently is too nested, correct?

Well I wouldn't say it's "too nested", but yes. In general I don't like it that closures are registered instead of module level functions (though I did end up using it myself in channel.py).

[nitori]

Forgot to address the following:

But realistically, you don't care about particular users, or if you do, you don't care about which channels they are in because you'd communicate with them directly via PRIVMSG. That means we have the semantic "Server <-1--n-> User" and "Server <-1--n-> Channel <-1--n-> User" relationships, even though technically Channel and User are many-to-many. The point is, however, that we only truly know which channel has which users and not which users is on which channels, because we only know of a limited subset of available channels, so a one-user-to-many-channels relationship would only be half true.

Well, commands like "!notify" or "!seen" might be interested in knowing in what channels are user is. At least from the bots perspective. I know that the bot will never have the full many-to-many information, because it will never on all channels and even then possibly never see all users (e.g. NickServ is usually hidden).

But I still think this is a better approach than having a set of users/members in each channel and having to "go through all channels" to find a specific user for e.g. the mentioned commands above. Or at least, I don't see any advantage/disadvantage of one of them over the other.

[FichteFoll]

Well, commands like "!notify" or "!seen" might be interested in knowing in what channels are user is.

!notify, not really. You either /whois the user to see if they are online at the time of the command's invocation, or you listen to join events (for the same or any different channel). You could take a shortcut by checking the channels you know of, still.

!seen, I wonder. First, it would depend on what the command is supposed to do exactly. Should it consider only the current channel? Then it doesn't matter. Should it consider all channels? Then, if the person is online, you'd see that in /whois. If they are not and had left or quit previously, you would have recorded that in a database.

That said, I'm okay with storing this data in a pseudo-relational database, although it seems like over-engineering because we'd never need it – and if we ever did, we could still iterate over all channels (just more slowly). Heck, we could even make it an actual database using in-memory sqlite. Initially, I was mostly concerened about the API.

[FichteFoll]

In general I don't like it that closures are registered instead of module level functions

I'll have to make a short write-up for this and ask a few other people about their opinion.

[FichteFoll]

I address all my curretnly remaining change requests (and more) in #47. This does not mean that this is done, however. Mode changes still aren't processed, for example. I don't know whether we want to hold this off until everything works, however.

(also note to self)