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
Add documentation and tests for framework #101
Conversation
Somehow forgot to run the tests before committing, oops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, looks good, just some nitpicks for the most part.
src/framework/configuration.rs
Outdated
/// use serenity::model::GuildId; | ||
/// | ||
/// client.with_framework(|f| f.configure(|c| c | ||
/// .blocked_guilds(vec!(GuildId(7), GuildId(77)).into_iter().collect()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we indent this by only 4 spaces? And for the others.
/// client.with_framework(|f| f.configure(|c| c
/// .blocked_guilds(vec![GuildId(7), GuildId(77)].into_iter().collect())));
vec![]
syntax is preferred.
src/framework/configuration.rs
Outdated
/// client.with_framework(|f| f | ||
/// .command("ping", |c| c.exec_str("pong!")) | ||
/// .configure(|c| c.disabled_commands( | ||
/// vec!("ping").into_iter().map(|x| x.to_owned()).collect()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 indentation here. Maybe we should bind the disabled commands prior to with_framework
and move it in to the disabled_commands
call? There's not a clean way to write this inlined.
src/framework/configuration.rs
Outdated
pub fn owners(mut self, user_ids: HashSet<UserId>) -> Self { | ||
self.owners = user_ids; | ||
|
||
self | ||
} | ||
|
||
/// Sets the prefix to respond to. This can either be a single-char or | ||
/// multi-char string. | ||
/// multi-char string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're at it this reads incorrectly, can this be changed to "A prefix can be a string slice of any non-zero length."?
src/framework/mod.rs
Outdated
/// client.with_framework(|f| f | ||
/// .bucket("basic", 2, 10, 3) // 3 uses per 10 seconds with 2 second delay | ||
/// .command("ping", |c| c | ||
/// .bucket("basic") // Use bucket defined above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd read better if the user knew beforehand what the code was doing, It looks like it might generate oddly in the docs and end up spanning across a second line like this. Create a bucket which allows for 3 uses per 10 seconds with a 2 second delay, and then apply the bucket to the "ping" command.
?
src/framework/mod.rs
Outdated
/// client.with_framework(|f| f | ||
/// .simple_bucket("simple", 2) // 2 second delay between uses | ||
/// .command("ping", |c| c | ||
/// .bucket("simple") // Use bucket defined above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
src/framework/mod.rs
Outdated
/// # fn main() { | ||
/// # use serenity::Client; | ||
/// # let mut client = Client::login("token"); | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This empty non-#
line - and the one above - should show some awkward top padding in the generated code example. Can we #
them?
src/framework/mod.rs
Outdated
/// | ||
/// # Examples | ||
/// | ||
/// Using before to log command usage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to wrap "before" in an inline code block to make it clear it isn't a grammatical error.
src/framework/mod.rs
Outdated
/// | ||
/// # Examples | ||
/// | ||
/// Using after to log command usage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
src/framework/mod.rs
Outdated
/// ```rust | ||
/// # use serenity::Client; | ||
/// # let mut client = Client::login("token"); | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with above, if I'm not mistaken this empty line will cause extra padding at the top of the codeblock -- I think it'd look cleaner if this were /// #
, instead of removing the line altogether.
src/framework/mod.rs
Outdated
/// .before(|ctx, msg, cmd_name| { | ||
/// if let Ok(channel) = msg.channel_id.get() { | ||
/// if !channel.is_nsfw() { | ||
/// return false; // Don't run unless in nsfw channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe all but two areas of the documentation prefer the comment to be above the code, rather than on the same line. Can this be moved above the if !channel.is_nsfw()
?
Work in progress: