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

Better CommandHandler #152

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

Zelaux
Copy link

@Zelaux Zelaux commented Aug 22, 2023

Added:

  1. possibility to use spaces in param names
  2. possibility to write required params after optional
  3. variadic params may not be at the end
  4. better exceptions

Added Tests for params parser and splitter

@Anuken
Copy link
Owner

Anuken commented Aug 22, 2023

General comments:

  • Is all this complexity really necessary? 4 extra files for command parsing...?
  • Are these changes really necessary at all?
    • Spaces in param names is a minor convenience, yes.
    • Why? This is very confusing. As a comparison, programming languages don't support this in methods, and for good reason. (C#: Optional parameters must appear after all required parameters)
    • Why? This, again, seems confusing to use, and, again, is not allowed in modern programming languages. (C#: A params parameter must be the last parameter in a formal parameter list)
  • Formatting is completely off. See Mindustry's CONTRIBUTING.md.
    • You're adding spaces before braces.
    • SCREAMING_SNAKE_CASE should not be used.
    • Use wildcard imports.

@Zelaux
Copy link
Author

Zelaux commented Aug 22, 2023

  1. All these 4 classes can be placed in one CommandHandler, but then it will turn into garbage
  2. Example command /rate [player...] <score> by default player is my self, or other person, or server
    It's like overloding functions
  3. Fixed

@Anuken
Copy link
Owner

Anuken commented Aug 22, 2023

  1. Sure, but there's a trivial workaround: just put [player...] at the end. I don't think it's worth massive command handler changes just so a parameter can be reordered.

@Zelaux
Copy link
Author

Zelaux commented Aug 22, 2023

It seems to me from the point of view of use it is more logical to write like this
/rate [player...] <score>
it is more logical to first write who I want to rate
But if a want, I can just write a score

Another command
/pay <player...> <coins amount>

@Zelaux
Copy link
Author

Zelaux commented Aug 22, 2023

Also I guess my parser and splitter creates less strings

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