-
Notifications
You must be signed in to change notification settings - Fork 119
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
feat(macro): add group macro #267
base: current
Are you sure you want to change the base?
Conversation
Adds the poise::group macro, which lets you group commands in a struct. Use `MyStruct::commands()` to get a vec of the commands.
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.
Looks okay, just a few nitpicks that would improve readability before I dive too deep into it.
Removes a redundant use std::vec;
Moves the body of group into a separate function and removes match in favour of ?
More direct comparison, without iter, map and format!.
Used a match over iterator items. Will now correctly handle paths with more than 2 segments.
Instead of quote!(), use proc_macro2::TokenStream::new()
Gives better name to command. Adds necessary use's.
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.
LGTM, just a couple minor fixes and I'll merge
Removes hardcoded guild id and registers commands globally. I had copied some test code and left that in.
macros/src/lib.rs
Outdated
|
||
let group_args = <command::GroupArgs as darling::FromMeta>::from_list(&args)?; | ||
|
||
// let item_impl = syn::parse_macro_input!(input_item as syn::ItemImpl); |
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.
// let item_impl = syn::parse_macro_input!(input_item as syn::ItemImpl); |
|
||
if ctx_type_with_static.is_none() { | ||
let context_type = match function.sig.inputs.first() { | ||
Some(syn::FnArg::Typed(syn::PatType { ty, .. })) => Some(&**ty), |
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.
Why return Some
here just to unwrap it, there isn't a branch that returns None?
#item_stream | ||
); | ||
} | ||
|
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.
Handle ctx_type_with_static
being none here?
Ok(()) | ||
} | ||
} | ||
|
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 your add a test for multiple commands, and a test for no commands in a group?
Adds the poise::group macro, which lets you group commands in a struct.
Use
MyStruct::commands()
to get a vec of the commands.See the doc of poise::group for more details.
Needs more documentation and examples.