Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scaling Dispike to Large, Organized Projects - Questions and Concerns #46

Closed
EthanC opened this issue Jul 22, 2021 · 8 comments · Fixed by #47
Closed

Scaling Dispike to Large, Organized Projects - Questions and Concerns #46

EthanC opened this issue Jul 22, 2021 · 8 comments · Fixed by #47
Labels
Fixed in the Next Release This issue will be resolved in the next release.

Comments

@EthanC
Copy link

EthanC commented Jul 22, 2021

I'd like to preface this discussion with thanks, Dispike is an impressive project and I look forward to seeing its continued growth.

As Dispike nears its first stable release, I've begun to scaffold my first project that will be based upon it. It's here that I've run into concerns regarding how a "large-scale" Dispike project should be organized; however, I'm unsure whether this is a matter of library design or lack of understanding on my end.

As it stands, an ideal Dispike bot (as provided by the example) is structured as follows:

bot.py

bot = Dispike(...)

@bot.interaction.on("...")
async def CommandA(...):
    return DiscordResponse(...)

@bot.interaction.on("...")
async def CommandB(...):
    return DiscordResponse(...)

@bot.interaction.on("...")
async def CommandC(...):
    return DiscordResponse(...)

This is a clean and simple approach to interfacing with the library, but it quickly becomes problematic at scale and I'm unsure how to resolve it. Because the bot instance is used in the decorator, using classes (and accessing self for global variables) is not an option(?) As you could imagine, this is less than ideal when you have several commands or commands with large amounts of logic.

Discord.py provides a solution to this as follows:

.../commandGroupA.py

from discord.ext.commands import command

class ExampleBot:
    @command(name="commandA")
    async def CommandA(...):
        await ctx.send(...)

    @command(name="commandB")
    async def CommandB(...):
        await ctx.send(...)

    @command(name="commandC")
    async def CommandC(...):
        await ctx.send(...)

By importing command and using it in the decorator, we do not need to access the bot object. This allows for commands (interaction) to be defined in various files across the project without having to avoid circular imports and classes.

An example of what this could look like for Dispike is as follows:

bot.py

bot = Dispike(...)

bot.run(...)

.../commandGroupA.py

from dispike import interaction

class CommandGroupA:
    @interaction.on("commandA", eventType=EventTypes.COMMAND)
    async def CommandA(...):
        return DiscordResponse(...)

    @interaction.on("commandB", eventType=EventTypes.COMMAND)
    async def CommandB(...):
        return DiscordResponse(...)

    @interaction.on("commandC", eventType=EventTypes.COMMAND)
    async def CommandC(...):
        return DiscordResponse(...)

.../.../commandGroupB.py

from dispike import interaction

class CommandGroupB:
    @interaction.on("commandD", eventType=EventTypes.COMMAND)
    async def CommandD(...):
        return DiscordResponse(...)

    @interaction.on("commandE", eventType=EventTypes.COMMAND)
    async def CommandE(...):
        return DiscordResponse(...)

    @interaction.on("commandF", eventType=EventTypes.COMMAND)
    async def CommandF(...):
        return DiscordResponse(...)

I'd appreciate any input you may have on this @ms7m! It's not a groundbreaking issue by any means, but I'm hopeful that you'll have a solution that I'm overlooking that may not even require changes to the library.

Perhaps the example bot could be updated to demonstrate the best practices?

Thanks!

@ms7m
Copy link
Owner

ms7m commented Jul 24, 2021

I agree 100% with the concern on scaling this out -- I first noticed it when trying to develop the sample bot.
This will definitely lead to a little bit of a rewrite, and I'll try to develop some ideas over the weekend.


Edit: I'm gonna push a couple of ideas on the dev branch to get this rolling, this is what it'll probably look like

testing_commands.py

from dispike import Dispike
from dispike import interactions
from dispike.response import DiscordResponse
from dispike.register.models import DiscordCommand
import typing


class SampleCommandCollection(interactions.EventCollection):
    def __init__(self, *args, **kwargs):
        # if needed
        pass

    @staticmethod
    def command_schemas() -> typing.List[
        typing.Union[interactions.PerCommandRegistrationSettings, DiscordCommand]
    ]:
        return [
            DiscordCommand(
                name="testcommands",
                description="return simple command",
                options=[],
            )
        ]

    def registered_commands(self) -> typing.List[typing.Callable]:
        return [self.sample_interactions]

    # COMMANDS

    @interactions.on("testcommand")
    async def sample_command(*args, **kwargs) -> DiscordResponse:
        ...

    @interactions.on("testcommand2")
    async def sample_command_2(*args, **kwargs) -> DiscordResponse:
        ...

main.py

bot = Dispike(
    client_public_key=...,
    application_id=...,
    bot_token=...,
)

bot.register_collection(collection=SampleCommandCollection, initialze_on_load=True)

ms7m added a commit that referenced this issue Jul 24, 2021
- move EventHandler classes to the main Dispike object
- start work for allowing interactions.EventCollections to be registered with the main object
- remove unneeded imports
@mrshmllow
Copy link
Contributor

When writing the components, i played around with the idea of having more advanced commands having their own full class, with both the main handler and interaction handlers. But that is actually something that a dev could do themselves

@ms7m ms7m mentioned this issue Jul 27, 2021
@EthanC
Copy link
Author

EthanC commented Aug 1, 2021

@ms7m I had some time to try out the changes from #47, very happy with the progress and looking forward to the release. I believe all of my concerns have been resolved, though there's just one thing left that I'm unsure of and would like your input.

First, a minimal example for context...

./bot.py

from commands import ExampleCollection

class ExampleBot:
    def Run(self):
        bot = Dispike(...)

        # self.variable = "Anything"

        bot.register_collection(collection=ExampleCollection)

        bot.run(5000)

if __name__ == "__main__":
    ExampleBot.Run(ExampleBot)

./commands/__init__.py

from .example import ExampleCollection

./commands/example.py

class ExampleCollection(EventCollection):
    def command_schemas():
        return [
            PerCommandRegistrationSettings(
                schema=DiscordCommand(name="example", description="Example Command.", options=[]),
                guild_id=...,
            )
        ]

    @interactions.on("example")
    # async def ExampleCommand(self, ctx: IncomingDiscordInteraction) -> DiscordResponse:
    async def ExampleCommand(ctx: IncomingDiscordInteraction) -> DiscordResponse:
        # print(self.variable)

        return DiscordResponse(content=f"Hello, World!")

This example allows for the exact kind of structure that I described in the initial Issue. The only caveat I've found is that in the command handler (ExampleCommand()), self cannot be accessed. Am I missing something or is this currently not possible?

Ideally, values could be defined in the ExampleBot class and then accessed in commands. See the commented lines for an example.

@EthanC
Copy link
Author

EthanC commented Aug 1, 2021

Unrelated to the problem described in the above comment.

Using the changes from #47, I do not believe that it's possible to send a DeferredResponse in an EventCollection as the bot object no longer exists. Correct me if I'm wrong, but perhaps a way of solving this would be as follows...?

@interactions.on("example")
async def ExampleCommand(ctx: IncomingDiscordInteraction) -> DeferredResponse:
    return DeferredResponse(original_context=ctx, new_message=DiscordResponse(...))

@ms7m
Copy link
Owner

ms7m commented Aug 1, 2021

.register_event_collection function has an argument to help you initialize with arguments.. So in theory you should be able to do

 bot.register_collection(collection=ExampleCollection, initalization_arguments={"bot": bot}, initialze_on_load=True)

You can also do

_collection = ExampleCollection(bot=bot)
bot.register_collection=(_collection)

All of these methods assume that you have added __init__ to your collection.


@ms7m
Copy link
Owner

ms7m commented Aug 1, 2021

Actually I've just tried it myself, and there was a bug that dispike couldn't find any methods inside the collection that have a interactions.on decorator.

I've just pushed a change to the dev branch, so go ahead and pull/install and try everything above again.

@EthanC
Copy link
Author

EthanC commented Aug 2, 2021

Perfect, I think that's all of my scale concerns resolved! I'll get to building with the dev branch and report any further issues or concerns that I run into.

@ms7m ms7m added the Fixed in the Next Release This issue will be resolved in the next release. label Aug 7, 2021
@ms7m ms7m linked a pull request Aug 9, 2021 that will close this issue
@EthanC
Copy link
Author

EthanC commented Aug 28, 2021

The initalization_arguments argument for register_collection is misspelled, it should be initialization_arguments.

@ms7m ms7m closed this as completed in #47 Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in the Next Release This issue will be resolved in the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants