shizmob / pydle

An IRCv3-compliant Python 3 IRC library.
BSD 3-Clause "New" or "Revised" License
154 stars 47 forks source link

Decorator-based callback registration #145

Closed Metadorius closed 2 years ago

Metadorius commented 4 years ago

Working on some Discord-IRC bot project I noticed that pydle doesn't support this wonderful style of callback registration that's used by discord.py:

import discord

client = discord.Client()

@client.event
async def on_ready():
    print('We have logged in as {0.user}'.format(client))

@client.event
async def on_message(message):
    if message.author == client.user:
        return

    if message.content.startswith('$hello'):
        await message.channel.send('Hello!')

client.run('your token here')

(taken from https://discordpy.readthedocs.io/en/latest/quickstart.html) Would it be possible to implement this in the same manner in pydle officially so I won't need to build up something custom on top of the lib?

theunkn0wn1 commented 4 years ago

Pydle uses an inheritance-callback method for firing user events, see https://pydle.readthedocs.io/en/latest/usage.html#adding-functionality

The idea being you subclass the Pydle client and override / extend the public API events needed, and the pydle backend will invoke the public API. Changing this now would be a radical API change, and I don't quite see the benefits their syntax gives that outweigh such a significant API change.

Thinking about how to implement this as an alternate API to pydle, it is possible to create a Feature class that implemented all the public API functions as well as this def self.event(func: Callable) -> Callable: method. My downstream Pydle bot does something similar with its commands implementation. The major difference there is that implementation has a fixed signature, whereas here it would have a variable signature depending on the event being hooked.

TL;DR Its possible to implement this kind of syntax on top of the existing Pydle interface, though its benefits remain unclear. Implementing a completely secondary interface for Pydle is not conducive to maintainability, and replacing Pydle's current interface would be breaking.

Metadorius commented 4 years ago

The benefits are simple - it is a bit more flexible than it is right now. Decorator-based callback assignment allows to elegantly write event handlers that depend on objects not available in pydle subclass. For exampe in my project I have a main class that has a handful of objects, including discord.py and pydle class instances, and I need to implement functionality in pydle class object that would require pydle to be aware of all those objects in main class if I were to not use something like a decorator to register events from the main class. I can supply some code examples later.

theunkn0wn1 commented 4 years ago

That would be appreciated, having some concrete examples will help me better picture the use case.

theunkn0wn1 commented 4 years ago

Looking at discord.py's documentation, they actually have two mechanisms for registering event handlers. The first, which is requested in the OP, is a decorator documented here The second, is the route Pydle takes, and involves subclassing documented here for discord.py and here in pydle.

Its still not immediately clear what benefits this provides, I would appreciate some examples that highlight the strengths of the decorator approach.

Given my work towards #15 requiring a breaking API change anyways, this may be a great feature to tie alongside the next major release.

Metadorius commented 4 years ago

@theunkn0wn1 So here's the example from my current project. I'm writing a Discord-IRC bot with additional functionality. The structure of the bot is such that there is a main class which has config, Discord client, IRC client and some other class instances as instance fields. Here's the example of using the decorator-based approach for pydle (it's not pretty in implementation in IRCClient but it works):

class DiscordCnCNetBot(object):

    def __init__(self, config_path: str = 'config.json', event_loop=None):

        # snip

        self.irc_client = IRCClient(
            nickname=self.config.irc_name,
            eventloop=self.event_loop)
        self.setup_irc_client()

        self.discord_client = commands.Bot(
            command_prefix=self.config.discord_prefix,
            loop=self.event_loop)
        self.setup_discord_client()

    def setup_irc_client(self):

        # snip

        @self.irc_client.event_handler
        async def on_message(channel, sender, message):
            """Forward IRC message to Discord channel."""
            if sender == self.irc_client.nickname:
                return

            # clean up some garbo
            trimmed_msg = message[3:]
            if self.config.discord_message_channel:
                msg_channel = self.discord_client.get_channel(self.config.discord_message_channel)
                await msg_channel.send(f"**<{sender}>** {trimmed_msg}")

(taken from https://github.com/Metadorius/cncnet-discord-bot/blob/master/discord_cncnet_bot.py#L58) Ideally lib users won't need to subclass pydle.Client at all which is one benefit. Other benefit that you can use any field, variable, method or function that's available in context. If I were to implement it using inheritance like the official doc says, I'd need to implement a class which would duplicate half of the fields from main class (like those for config instance, for discord.py instance; and those are not the only fields as the functionality would be bigger than simply bridging) an would be aware of all other objects, which just doesn't seem nice from architectural viewpoint.

Metadorius commented 4 years ago

So, any update, thoughts or plans on the matter?

theunkn0wn1 commented 4 years ago

I plan on adding this feature.

Metadorius commented 3 years ago

Any luck? No rush, just asking :)

theunkn0wn1 commented 2 years ago

Sorry for the delay, College has not been kind. implemented in #165