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

Command Parser #39

Merged
merged 11 commits into from
Aug 20, 2024
Merged

Command Parser #39

merged 11 commits into from
Aug 20, 2024

Conversation

user622628252416
Copy link
Contributor

Regarding #15, I implemented a tree-style command parser, as well as the /help command for reference.

There is a central CommandDispatcher instance, that is contained inside of a OnceLock (pumpkin::commands::DISPATCHER), from where all commands are dispatched. There is currently no access to the level/world the command is executed in, so what a command can actually do is quite limited. This is more of a temporary solution.

@Snowiiii
Copy link
Owner

Hey, Mostly looks good 👍 . Could you may fix all Clippy warns ?. I also just pushed a commit which fixes the one warn in packet_encoder

@user622628252416
Copy link
Contributor Author

I fixed all the warns that I could fix. I did have to suppress two dead_code warns tho because no command needing literals is implemented as of now.

@Snowiiii
Copy link
Owner

Snowiiii commented Aug 20, 2024

I fixed all the warns that I could fix. I did have to suppress two dead_code warns tho because no command needing literals is implemented as of now.

Great. I just don't really like that in help command, Commands are send multiple times if they have multiple arguments. I find that very confusing. CI is btw unhappy (you have to format the code)

@user622628252416
Copy link
Contributor Author

I agree about the help command. Sending multiple lines is more of a temporary solution until someone implements merging all the paths on the tree into a single line (which is doable but not without some complexity). I added a todo 👍

@Snowiiii
Copy link
Owner

Okay. Everything other looks good. Thank you 👍

@Snowiiii Snowiiii merged commit 589d258 into Snowiiii:master Aug 20, 2024
1 check passed
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.

3 participants