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

Implement command groups #40

Merged
23 commits merged into from Dec 13, 2016
Merged

Implement command groups #40

23 commits merged into from Dec 13, 2016

Conversation

ghost
Copy link

@ghost ghost commented Dec 11, 2016

No description provided.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Overall I like the proposed functionality. 👍

Overall nit: one blank line before and after logic blocks (if, if-let, match, etc) please

.group("emoji", |g|
.command("cat", |c| c
.exec_str(":cat:")
.required_permissions(permissions::SEND_MESSAGES))
Copy link

Choose a reason for hiding this comment

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

It was probably a mistake of mine to use this permission as an example previously; maybe ADMINISTRATOR?

min_args: None,
max_args: None,
required_permissions: Permissions::empty()
}));
Copy link

Choose a reason for hiding this comment

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

This is used elsewhere as well. Maybe Command could use a new method with exec as the argument and setting everything else listed here as the defaults.

pub fn on<F, S>(mut self, command_name: S, f: F) -> Self
where F: Fn(&Context, &Message, Vec<String>) + Send + Sync + 'static,
S: Into<String> {

Copy link

Choose a reason for hiding this comment

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

nit: stray blank line

pub fn command<F, S>(mut self, command_name: S, f: F) -> Self
where F: FnOnce(CreateCommand) -> CreateCommand,
S: Into<String> {

Copy link

Choose a reason for hiding this comment

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

nit: stray blank line

use ::model::Permissions;
use std::sync::Arc;

pub struct CreateGroup(pub CommandGroup);
Copy link

Choose a reason for hiding this comment

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

CreateGroup could use an example


permissions_fulfilled = perms.contains(command.required_permissions);
permissions_fulfilled = perms.contains(command.required_permissions);
Copy link

Choose a reason for hiding this comment

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

Side-note: the indentation here has gotten a little insane.

Note-to-self: revise this later

required_permissions: Permissions::empty()
}));
if !self.groups.contains_key("Ungrouped") {
self.groups.insert("Ungrouped".to_string(), Arc::new(CommandGroup {
Copy link

Choose a reason for hiding this comment

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

Arc::new(CommandGroup::default()) to simplify things a bit

@@ -395,7 +422,31 @@ impl Framework {
where F: FnOnce(CreateCommand) -> CreateCommand,
S: Into<String> {
let cmd = f(CreateCommand(Command::default())).0;
self.commands.insert(command_name.into(), Arc::new(cmd));
if !self.groups.contains_key("Ungrouped") {
self.groups.insert("Ungrouped".to_string(), Arc::new(CommandGroup {
Copy link

Choose a reason for hiding this comment

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

and here

}

self
}
Copy link

Choose a reason for hiding this comment

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

Although I don't think semver requires this for versions < 1.0.0, it'd be nice to remove this only once we hit v0.2.0, rather than the v0.1.2 that the next version is planned to be. Is it possible to keep this?

min_args: None,
max_args: None,
required_permissions: Permissions::empty()
}));
Copy link

Choose a reason for hiding this comment

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

This could also make use of the proposed Command::new from above

@ghost ghost merged commit daf92ed into serenity-rs:master Dec 13, 2016
@ghost ghost deleted the command-groups branch December 13, 2016 19:27
@ghost ghost mentioned this pull request Dec 13, 2016
@hsiW hsiW added the enhancement An improvement to Serenity. label Dec 29, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement to Serenity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant