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

feat(macro): add group macro #267

Open
wants to merge 16 commits into
base: current
Choose a base branch
from

Conversation

TitaniumBrain
Copy link

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.

Adds the poise::group macro, which lets you group commands in a struct.

Use `MyStruct::commands()` to get a vec of the commands.
Copy link
Member

@GnomedDev GnomedDev left a 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.

macros/src/lib.rs Outdated Show resolved Hide resolved
macros/src/lib.rs Show resolved Hide resolved
macros/src/lib.rs Outdated Show resolved Hide resolved
macros/src/lib.rs Outdated Show resolved Hide resolved
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.
macros/src/lib.rs Show resolved Hide resolved
macros/src/lib.rs Outdated Show resolved Hide resolved
macros/src/lib.rs Outdated Show resolved Hide resolved
macros/src/lib.rs Show resolved Hide resolved
macros/src/lib.rs Outdated Show resolved Hide resolved
macros/src/lib.rs Outdated Show resolved Hide resolved
macros/src/command/mod.rs Show resolved Hide resolved
macros/src/lib.rs Show resolved Hide resolved
Copy link
Member

@GnomedDev GnomedDev left a 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

macros/src/lib.rs Outdated Show resolved Hide resolved
macros/src/lib.rs Outdated Show resolved Hide resolved
TitaniumBrain and others added 4 commits June 14, 2024 14:56
Removes hardcoded guild id and registers commands globally.

I had copied some test code and left that in.

let group_args = <command::GroupArgs as darling::FromMeta>::from_list(&args)?;

// let item_impl = syn::parse_macro_input!(input_item as syn::ItemImpl);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 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),
Copy link
Member

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
);
}

Copy link
Member

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(())
}
}

Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants