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

feat: implement simple plugins #3

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

feat: implement simple plugins #3

wants to merge 7 commits into from

Conversation

Sharp-Eyes
Copy link
Collaborator

@Sharp-Eyes Sharp-Eyes commented Jan 2, 2023

This PR adds simple plugins and extensions, which can be used to place commands and listeners in separate files. Furthermore, plugins can be loaded, unloaded and reloaded as desired, to hot-reload code, etc. This should lead to an overall better development experience. Registering sub-modules to a plugin is a planned feature, but will be delayed to a future PR.

@Sharp-Eyes Sharp-Eyes added the enhancement New feature or request label Jan 2, 2023
Copy link

@EmmmaTech EmmmaTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all, pretty pogger PR.

sail/impl/plugin.py Outdated Show resolved Hide resolved
sail/impl/plugin.py Outdated Show resolved Hide resolved
import sail

logging.basicConfig()
logging.getLogger().setLevel(logging.DEBUG)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you set the level directly in the basicConfig function?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may not remember this correctly but I'm pretty sure setting it in basicConfig is global and for all loggers as opposed to just the logger in this file.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the point? This is an example, not a module part of the library.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getLogger() without an arg also uses the global logger, so that doesn't change the behaviour either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, though I'm probably leaving out the log setup altogether when I finish up this example (note how there's no comments explaining anything yet), as it's only there as I needed it to debug stuff.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeeted this in c5d2b41. I'll add a bit of logging explanation later when I get around to properly documenting this example, as plugins do actually support setting per-instance loggers.

@thesadru
Copy link

thesadru commented Jan 3, 2023

How will state be managed? 🤔

@Sharp-Eyes
Copy link
Collaborator Author

Sharp-Eyes commented Jan 3, 2023

How will state be managed? 🤔

Though I probably misunderstood the question -- as of right now, not a single model is stateful, and I'm kinda inclined to keep it that way (not that Eludris currently has anything that warrants maintaining state locally). The client can be accessed through the Plugin.client property, and any interaction with Eludris can be done from there.
The main reason I want to keep away from stateful models is so that one can easily replace the (as of yet nonexistent) cache with e.g. an external redis/keydb cache, etc.

@thesadru
Copy link

thesadru commented Jan 3, 2023

Since it's supposed to be inspired by Hikari I'm talking about stuff like injections. You speak of replacing the cache but how will you get the same db client across all plugins?

@Sharp-Eyes
Copy link
Collaborator Author

Ah like that. Yeah, as of right now the only real typesafe way would be to create the cache instance in its own file and quite literally import it into all files that need it. As for possible solutions, I'm indeed leaning towards some flavour of dependency injection, though some kind of simple datastore could do the trick as well.

@thesadru
Copy link

thesadru commented Jan 3, 2023

I think it's gonna be very hard to make a type-safe datastore. Injections are the most elegant solution IMO. Although I suppose you could inject a datastore lol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants