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

Command module loading #3

Open
kaylendog opened this issue May 13, 2021 · 4 comments · May be fixed by #2
Open

Command module loading #3

kaylendog opened this issue May 13, 2021 · 4 comments · May be fixed by #2
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@kaylendog
Copy link
Contributor

From #2, @kokumaji added support for loading commands directly from the file system. This has somepotential ramifications - nothing inherently wrong with doing it, but it does come at the cost of some type safety.

We can either:

  • Stick with this and add some type checks on the loaded module to see if it's valid.
  • Switch to declaring commands explicitly at compile time.

Both sides have their pros and cons, but needs discussing to work out what to do.

@kaylendog kaylendog added good first issue Good for newcomers help wanted Extra attention is needed labels May 13, 2021
@kokumaji
Copy link

I talked to @vladfrangu about this and they suggested https://sapphiredev.github.io/framework as an alternative, the current implementation is really just something I'm familiar with that works. Of course a proper command handler is a better approach

@kaylendog
Copy link
Contributor Author

I talked to @vladfrangu about this and they suggested https://sapphiredev.github.io/framework as an alternative, the current implementation is really just something I'm familiar with that works. Of course a proper command handler is a better approach

If Vlad wants to use his own library, he can. Given there's still like, no documentation for Sapphire aside from the auto-generated typedocs, it's not something I would suggest we use.

@vladfrangu
Copy link

Leaving aside the fact it's not my library, I don't see why that affects anything, when the entire library is so small and extensible, it only adds small helper stones... THere's also a bunch of bots already using it, whose code is OSS, so...

@kaylendog
Copy link
Contributor Author

Leaving aside the fact it's not my library, I don't see why that affects anything, when the entire library is so small and extensible, it only adds small helper stones... THere's also a bunch of bots already using it, whose code is OSS, so...

Oh please, we know you contribute to it. The lack of documentation is like a really big problem - reading OSS code is only going to get some of us so far.

@kaylendog kaylendog linked a pull request May 13, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants