search

Microsoft-community / Dotsimus-TS

1 stars 1 forks source link

Existing code updates #3

Closed cadenkoj closed 2 years ago

cadenkoj commented 3 years ago

This is where Koji puts stuff that needs reviewed and merged

lost-in-action commented 3 years ago

Alright, if @BeakerThe1st is ok we can merge the pull request. There is 1 thing: subcommands need to be made so it doesn't need the parent command code to be modified if you want to add a subcommand. me, uptime, ping, restart, usage are all in about.ts, it would be better to code in a way that it doesn't need About.ts to be changed

So if you have subcommands, then the parent must be aware of it: maybe a new member children containing a list of subcommands,

cadenkoj commented 3 years ago

Alrighty

cadenkoj commented 3 years ago

why not MIT like the old dot?

I didn't pick the license, BeakerThe1st did

cadenkoj commented 3 years ago 
const tokenENV = "DISCORD_TOKEN",
devTokenENV = "DISCORD_TOKEN_DEV"

Why not add those to the env variables instead of providing them here? It would be cleaner and better to do this and it will be easier to set up

If you mean in .env, they are, there are just some pros to doing this

cadenkoj commented 3 years ago

Latest comments are because they didn't show up on my phone 😅

BeakerThe1st commented 3 years ago

why not MIT like the old dot?

I didn't pick the license, BeakerThe1st did

I don't recall including a license...

cadenkoj commented 3 years ago

Thank you for all of the feedback @BeakerThe1st, I will work on everything that should be changed and work on improvements soon.

Jasius commented 3 years ago

I don't see why there needs to be DISCORD_TOKEN_DEV and DISCORD_TOKEN env vars, ideally the production token would only be present on the production environment

You're right, it's unnecessary. It's just one of the relics from the early implementation as I used to host prod on my local machine from time to time which helped to pinpoint issues better.