Moving Converters and the Source Cog into botcore

[GDWR]

While working on Sir Robin, we were porting the Source command as we wanted that functionality.

During this, we needed to duplicate the converters.py and also the source.py cog. To me it feels like this would be a good candidate for the first cog implemented in botcore.

[GDWR]

Also, if this is done an issue should be created on Sir Robin to change how it is being used.

[wookie184]

Most aspects of the source command should be the same across bots, although python-discord/bot also needs to support tags. Having an extensible cog that would allow bot to hook in to add support for tags would be nice, although might also overcomplicate things. Thoughts?

[GDWR]

It seems like the repository is already structured in a way that this would be the plan, exts/init.py

An example of what I would be expecting can be found here. As an example of importing a cog that is not in the same file, to then be setup and loaded.

---

Define each ext/cog approach

This is a code snippet of how a project would use a botcore ext (cog)
bot/exts/source.py

from bot.bot import Bot


def setup(bot: Bot) -> None:
    """Set up the Source extension."""
    from botcore.exts import Source

    bot.add_cog(Source(bot))

It would also be extendible like so
bot/exts/source.py

from bot.bot import Bot

import botcore

class Source(botcore.exts.Source):
    """Extend the Botcore Source cog to delete the message after 5 seconds"""

     """
     Does this override the defined command from the base class?  or do they clash/error
         - Override, means we have the behaviour we want
         - Clash/Error, we will have to look for an alternative pattern maybe using a new 
         
            @precommand(name="source") 
            decorator that will be ran before the command, expecting you to return the `ctx` and params out
     
            @postcommand(name="source")
            decorator that will be ran after the command, with the parms being the functions return type
    """
    @commands.command(name="source", aliases=("src",))
    async def source_command(self, ctx: commands.Context, *, source_item: SourceConverter = None) -> None:
        """Display information and a GitHub link to the source code of a command, tag, or cog."""

        # Currently it doesn't actually return the send message, so this wouldn't work
        #    Additionally, passing in converted items will be finicky.
        message = await super().source_command(ctx, source_item=source_item)
        await message.delete(delay=5)
    


def setup(bot: Bot) -> None:
    """Set up the Source extension."""
    bot.add_cog(Source(bot))

So to me, the things that need to be ironed out if this approach is used are;

• Do we return messages sent? to allow subclasses to easily interact with the messages
  • If we do, what happens if there are multiple messages?
• How do we handle "Special" (Converters) params for subclasses

---

Subclassing botcore.BotBase approach

All that would be required is for your bot class to be a subclass of botcore.BotBase.

Currently, the botcore.BotBase.load_extensions allows for you to specify a module and it will load all extensions it holds.

@ChrisLovering suggested that we just add all the botcore extension (mostlikely to be configurable) within this method.

async def load_extensions(self, module: types.ModuleType) -> None:
    """Load all the extensions within the given module and save them to ``self.all_extensions``."""
    self.all_extensions = walk_extensions(module) + walk_extensions(botcore.exts)
    for extension in self.all_extensions:
            await self.load_extension(extension)

This would be a simple way to add all extensions but would remove the possibility of extensible usage of the cogs (which might be favourable for simplicity).

---

I favour the extensible / define each cog approach, although there might be some sweet in between. Where we combine the exts/cogs into a set using the cog name as the hash, preferring the non-botcore extension. Although this could be confusing/magic for users of the lib.

[wookie184]

Thanks for writing that up, I like the "Define each ext/cog" approach, although I'm not sure the precommand/postcommand hooks would add anything we can't do by overloading. Overloading would also give us more flexibility and let us reuse some of the existing code flow.

For example, to add the tag behaviour for bot, i'd propose that the subclass might look something like this. We'd overload each method that needs tag specific behaviour and implement the tag behaviour before delegating to the superclass methods for other stuff.

BotSourceType = Union[botcore.exts.Source.SourceType, TagIdentifier]

class BotSourceConverter(botcore.exts.Source.SourceConverter):
    @staticmethod
    async def convert(ctx: commands.Context, argument: str) -> BotSourceType:
        with contextlib.suppress(commands.BadArgument):
            return await super().convert(ctx, argument)

        ... # get tag
        escaped_arg = escape_markdown(argument)

        raise commands.BadArgument(
            f"Unable to convert '{escaped_arg}' to valid command{', tag,' if show_tag else ''} or Cog."
        )

class BotSource(botcore.exts.Source):
    @commands.command(name="source", aliases=("src",))
    async def source_command(self, ctx: commands.Context, *, source_item: BotSourceConverter = None) -> None:
        """Display information and a GitHub link to the source code of a command, tag, or cog."""
        embed = await self.build_embed(source_item)
        await ctx.send(embed=embed)

    async def get_source_link(self, source_item: BotSourceType) -> Tuple[str, str, Optional[int]]:
        if isinstance(source_item, TagIdentifier):
            ...
            return url, file_location, None
        return await super().get_source_link(source_item)

    async def build_embed(self, source_object: BotSourceType) -> Optional[Embed]:
        # maybe create a get_title_and_description() function to overload instead of this one
        # so the actual embed creation logic can be reused.
        if isinstance(source_object, TagIdentifier):
            ...
            return ...
        return await super().build_embed(source_object)

There may still be a small amount of code reuse, but we can split anything generic into separate functions if necessary to help with that.

[GDWR]

Ah, so overriding works like that?
I was writing both scenarios, where it doesn't correctly override and what solutions could be for that case.

This impl looks like a really good way of doing it.
It would require a lot of studying/referring to the base cog (for a developer of a project using the lib), but that can be detailed in their docstrings on the class. I don't think it is too complicated.

[wookie184]

Ah, so overriding works like that? I was writing both scenarios, where it doesn't correctly override and what solutions could be for that case.

Ooh, sorry, yeah. I didn't read what you wrote properly. I just tested and it seems to work how we'd want it to, i.e.

class A(Cog):
    @command()
    async def test(self, ctx):
        print("Test called")

class B(A):
    @command()
    async def test(self, ctx):
        print("Subclass test called")

bot.add_cog(B())

prints only Subclass test called when running the test command.

[GDWR]

So from that, we can gather that we don't need to have any (unusual) standard for writing cogs, as they can be efficiently and effectively for the library users.

Should this PR begin to be actioned, i.e. moving over the source command? so Robin & Bot can begin to use the botcore impl

[wookie184]

Should this PR begin to be actioned, i.e. moving over the source command? so Robin & Bot can begin to use the botcore impl

Yes, would you like to do this?

[GDWR]

Not at the moment, if in ~2 weeks it is still up for grabs I might pick this up