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

Commando rewrite #8

Merged
merged 109 commits into from
Sep 28, 2019
Merged

Commando rewrite #8

merged 109 commits into from
Sep 28, 2019

Conversation

sustained
Copy link
Collaborator

@sustained sustained commented Sep 1, 2019

This is not ready for merging nor review. But here it is anyway.

Todo

  • ES6-module-ise codebase:
    • Client
    • Commands
    • Services
    • Utilities
    • Jobs
  • Commando-ise all commands
  • Switch to .env files for configuration
  • Set up ESLint / Prettier
  • Move data to data/
    • Move banned words list
    • Extract docs-related data
    • ❓ Extract fields from !code and build dynamically for easier reading + modification
  • Add permission checks for moderation-related commands
  • Set up personal test server
    • Test all existing (and new) commands
      • !docs
      • !code
      • !enable-job
      • !disable-job
      • !job-info
      • !list-jobs
      • !add-ban-word
      • !list-ban-words
      • !remove-ban-word
  • Test jobs
    • BanJob
    • WarnJob
  • Jobs system improvements
    • I think there should be a Job class and that jobs should extend it, so as to align the API with Commando and how its commands work.
    • Ideally our "jobs" would also be able to be enabled/disabled/reloaded etc. at runtime.
    • Too many magic strings such as the role.name (Admin) and the channel name (spam). I think this stuff needs moving to configuration. Perhaps jobs can have their own configuration.
    • Right now jobs run for every message, no matter what. That needs fixing. We probably want to run it for other bots (after all, bots can misbehave) but definitely not if we are the message author.
    • It seems desirable to allow the system to prevent execution entirely for certain jobs, depending on the user's roles or permissions.

Things to take into consideration

  • Since there is no linting yet, our coding styles may not be entirely in alignment:
    • I had to try very hard to use single quotes (I use double) - I may have missed some
    • I had to try very hard to omit semi-colons (I use them) - I may have accidentally added some
    • I don't put space between function names and parentheses, like you - noticed this one late
  • The jobs system hasn't been rewritten
    • Also I totally didn't realise what it actually was and removed a bunch of code that will probably be readded. I thought e.g. ban and warn were just regular commands. I should have taken more time to understand the codebase! Your code will be re-added, don't worry.
  • None of this has been tested (yet)
    • I actually haven't even ran it, this is just a first pass 😄

Noteworthy changes

  • I have switched to ES6 modules project-wide.
    • I am using a cool library called esm which lets us do this without having to configure Babel.
  • I have switched to .env files instead of the config.json thing.
    • I am using dotenv to load the file into process.env.
  • I have moved data (e.g. ban-words.txt) out of src/ and into a new root data/.

A note on linting / coding style

I will set up linting soon (adhering to YOUR coding style, don't worry - I'm not that bad 😈).

This is generally how things are done with Commando.
Commando generates all of this stuff for us.
I mistakenly thought they were normal commands. Kind of jumped the gun there.
Commando provides a ping command which in my opinion achieves the same thing.
I'm not actually sure if this is even possible with Commando anyway, since you need to define all argument that will be passed?
@sustained
Copy link
Collaborator Author

@Elfayer Ready for review IMHO (no rush/pressure). 🙃


module.exports = new Discord.Client()
const {
OWNERS_IDS = '269617876036616193', // Default to @evan#9589
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't it be process.env.OWNERS_IDS from .env.example

Copy link
Collaborator Author

@sustained sustained Sep 2, 2019

Choose a reason for hiding this comment

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

Perhaps... but, well, the intent here was to avoid errors if the .env file is missing or if the OWNERS_IDS entry was missing.

It's just a fallback.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

const {
  OWNERS_IDS = '269617876036616193', // Default to @evan#9589
  COMMAND_PREFIX = '!',
} = process.env

Is basically equivalent to this:

let { OWNERS_IDS, COMMAND_PREFIX } = process.env
if (!OWNERS_IDS) OWNERS_IDS = '269617876036616193'
if (!COMMAND_PREFIX) COMMAND_PREFIX = '!'


import links from '../../../data/documentation'

module.exports = class DocsDocsCommand extends Command {
Copy link
Owner

Choose a reason for hiding this comment

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

DocsCommand?

Choose a reason for hiding this comment

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

Command (I've asked this before)

Copy link
Collaborator Author

@sustained sustained Sep 3, 2019

Choose a reason for hiding this comment

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

My own personal convention is:

NAME_OF_COMMAND_GROUP + NAME_OF_COMMAND + COMMAND.
  • The command name is docs.
  • The command group is docs (well by now it's actually documentation)
  • So it is DocsDocsCommand (should be DocumentationDocsCommand by now, as said - the group changed).

I think this command should be renamed to Vue anyway and that there should be a command per library.

So we are left with:

  • DocumentationVueCommand
  • DocumentationVueXCommand
  • DocumentationVueRouterCommand

Thoughts? @Elfayer

@sustained
Copy link
Collaborator Author

I think I'm happy with most of this now. I just really wanted to clean up that Job class and so that has been done. But I couldn't help sneak in a small new feature. My apologies, I suppose this is a nightmare to review.

@sustained
Copy link
Collaborator Author

sustained commented Sep 4, 2019

Updating the README is 100% my final change, unless changes are requested.

@Elfayer Elfayer merged commit 565b269 into Elfayer:master Sep 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants