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

Decouple commands object from the bot Client object #747

Merged
merged 3 commits into from
Feb 24, 2024

Conversation

ghost
Copy link

@ghost ghost commented Feb 21, 2024

ViBot [8.16.8]

Changelog

Changes

  • Moved commands await from the bot Client object and into lib/commands.js.
  • Refactored all possible references to it, including those using client.commands.
  • Some small cleanup along the way

@ghost ghost self-requested a review as a code owner February 21, 2024 18:45
@ghost ghost linked an issue Feb 21, 2024 that may be closed by this pull request
@Ragviswa Ragviswa added the Change A modification to something that exists label Feb 21, 2024
@Ragviswa Ragviswa assigned ghost Feb 21, 2024
Comment on lines +8 to +14
load() {
const files = readdirSync('./commands').filter(file => file.endsWith('.js'));
for (const file of files) {
const command = require(`../commands/${file}`);
commands.set(command.name, command);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than calling load, why not move this block into the top level of this file?

Copy link
Author

Choose a reason for hiding this comment

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

I tend to like explicitly saying "I'm initializing this," however I guess to me that only really matters if there's any necessary order to it and at least for now it just needs to be loaded before commands is used, so whenever it's 'require'd should be fine.

Copy link
Author

Choose a reason for hiding this comment

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

It turns out without a lot of refactoring this probably won't work because requiring commands inside of lib/commands.js causes several circular dependencies.

Take just addalt.js for instance. It requires ../utils.js, which requires ./commands/commands.js, which in turn requires ./lib/commands.js. Because of this require chain, lib/commands.js will not be finished & cause the whole require chain to break down.

Putting it in a function to call once lib/commands.js is not being constructed avoids the circular dependency issue.

Copy link
Contributor

@Ragviswa Ragviswa left a comment

Choose a reason for hiding this comment

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

Looks good to me, pretty much just file restructuring and a search + replace, so I'm not concerned :)

husky-rotmg and others added 3 commits February 24, 2024 11:49
Moved `bot.commands` into `lib/commands` and refactored all references to it. Made sure to include `client.commands` too for non-direct accesses
@Ragviswa Ragviswa force-pushed the husky/decoupling/bot-commands branch from ef72bd6 to 8aec176 Compare February 24, 2024 11:49
@Ragviswa Ragviswa merged commit 363b31f into master Feb 24, 2024
1 check passed
@Ragviswa Ragviswa deleted the husky/decoupling/bot-commands branch February 24, 2024 11:54
@Thomasc33 Thomasc33 unassigned ghost Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Change A modification to something that exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move bot.commands & loadCommands into a commands.js
2 participants