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

Check for command entity first in Command.checkMessage #183

Merged
merged 2 commits into from
Jul 31, 2024
Merged

Check for command entity first in Command.checkMessage #183

merged 2 commits into from
Jul 31, 2024

Conversation

Jisin0
Copy link
Contributor

@Jisin0 Jisin0 commented Jul 22, 2024

What

Minor improvement on Command.checkMessage to look for a command entity before trying to parse the command.

Impact

  • Are your changes backwards compatible? Yes
  • Have you included documentation, or updated existing documentation? N/A
  • Do errors and log messages provide enough context? N/A

@PaulSonOfLars
Copy link
Owner

Hi, thanks for raising!

I understand what the PR does, but it's not clear to me why it's necessary; is the existing logic causing issues? Are there any errors to be shared?

@Jisin0
Copy link
Contributor Author

Jisin0 commented Jul 22, 2024

There's no issue with the current setup. But it made more sense to check if there is a command entity before splitting and parsing the command, which I assume will do for all messages unnecessarily that reach the handler.

Copy link
Owner

@PaulSonOfLars PaulSonOfLars left a comment

Choose a reason for hiding this comment

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

Fair enough, hadn't thought about it that way. Good catch!

In the future, please add that to the PR description so there's some context to your changes :)

@PaulSonOfLars PaulSonOfLars enabled auto-merge (squash) July 31, 2024 17:42
@PaulSonOfLars PaulSonOfLars merged commit e67a260 into PaulSonOfLars:v2 Jul 31, 2024
3 checks 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.

2 participants