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

attempted to improve command organization #1236

Closed
wants to merge 7 commits into from

Conversation

CamHardy
Copy link
Contributor

@CamHardy CamHardy commented Oct 5, 2020

Reorganized the commands as best as I could discern, also made sure each command has the correct category property. Addresses issue #1189

@moruzerinho6
Copy link
Member

Jesus this is going to take a while to review, I was not expecting someone besides me to actually make this.

src/commands/games/leagueoflegends/champion.js Outdated Show resolved Hide resolved
src/locales/en-US/commands.json Outdated Show resolved Hide resolved
@metehus
Copy link
Member

metehus commented Oct 6, 2020

Btw, two more things:

  • What about those .DS_Store? i don't think it should be on the repository
  • I'm also thinking about the music category, with an idea of making music category for music commands(deezer, spotify, lastfm) and a player category for music playing (play, pause, nowplaying, etc)

@almeidx
Copy link
Member

almeidx commented Oct 6, 2020

  • What about those .DS_Store? i don't think it should be on the repository

Definitely not. It's a file used by macOS that stores custom attributes of its containing folder, such as the position of icons etc.

@CamHardy
Copy link
Contributor Author

CamHardy commented Oct 6, 2020

Gosh darn macOS adding hidden files. I'll remove them from the PR, and add .DS_Store to the gitignore

src/commands/configuration/language.js Outdated Show resolved Hide resolved
src/commands/configuration/prefix.js Outdated Show resolved Hide resolved
src/commands/misc/adorable.js Outdated Show resolved Hide resolved
src/commands/misc/dog.js Outdated Show resolved Hide resolved
src/commands/misc/fox.js Outdated Show resolved Hide resolved
src/commands/misc/imageoftheday.js Outdated Show resolved Hide resolved
src/commands/misc/shiba.js Outdated Show resolved Hide resolved
src/commands/moderation/blacklist.js Outdated Show resolved Hide resolved
src/commands/moderation/unblacklist.js Outdated Show resolved Hide resolved
src/commands/moderation/whyblacklisted.js Outdated Show resolved Hide resolved
Copy link
Contributor

@fersilva16 fersilva16 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@almeidx almeidx left a comment

Choose a reason for hiding this comment

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

I just noticed the target branch is master, not dev. However, changing it now will probably cause lots of conflicts. On the other hand, if we end up merging this into master instead, it will cause conflicts when merging dev -> master.

@moruzerinho6
Copy link
Member

It's on his fork, he can cancel the PR and create a new one targeting dev and them resolving the conflits.

@Doges
Copy link
Member

Doges commented Sep 8, 2021

We probably gonna use your PR as a base for categories on @switchblade-next, anyway, thanks for opening it.

@Doges Doges closed this Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-organize every command.
6 participants