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

Remove unnecessary lifetimes in CommandDispatcher and Command #419

Merged
merged 5 commits into from
Dec 26, 2024

Conversation

urisinger
Copy link
Contributor

@urisinger urisinger commented Dec 26, 2024

Description

This is a small change aiming to make the usage of CommandDispatcher a little bit easier by removing all of the unnecessary lifetimes. It changes all borrowed strings in CommandDispatcher to oned ones, it also replaces all refrences to dyn traits with Arcs. Both these changes have a small preformence impact, specifically when cloning, but it should be negligable considering they arent cloned alot. if needed i cant also swap the owned strings to Arc, which could be a bit faster with frequent cloning.

Testing

There currently aren't any unit tests for commands so i have no way of verifying whether everything worked as expected now or before.

Checklist

Things need to be done before this Pull Request can be merged.

  • Code is well-formatted and adheres to project style guidelines: cargo fmt
  • Code does not produce any clippy warnings: cargo clippy
  • All unit tests pass: cargo test
  • I added new unit tests, so other people don't accidentally break my code by changing other parts of the codebase. How?(not needed, this doesnt change any logic)

@urisinger urisinger changed the title Remove unneeded lifetimes by storing strings and dyn traits in CommandDispatcher and Command as owned values. Remove unneeded lifetimes in CommandDispatcher and Command Dec 26, 2024
@urisinger urisinger changed the title Remove unneeded lifetimes in CommandDispatcher and Command Remove unnecessary lifetimes in CommandDispatcher and Command Dec 26, 2024
@kralverde
Copy link
Contributor

Code looks good at a glance, haven't tested in-game though

@Snowiiii
Copy link
Member

Looks good.
Thanks @urisinger :D 👍

@Snowiiii Snowiiii merged commit 25f4834 into Pumpkin-MC:master Dec 26, 2024
9 checks passed
@Snowiiii
Copy link
Member

Code looks good at a glance, haven't tested in-game though

I did, Everything works fine

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