-
Notifications
You must be signed in to change notification settings - Fork 41
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
console: use the new string functions #1459
Conversation
bfd59c1
to
2a8c7a3
Compare
I've simplified the proposal to use only argument parser primitives, restoring the previous behavior, and it looks acceptable. However, this still adds a lot of code to libtrx. I'm hesitant to introduce it since the code, while functional, is currently unused and likely will remain unused due to the intended flexibility of the command arguments. Maybe the introduced functions should just become a couple of LMK what you think. |
It looks really good, but as you say maybe for now it's best to go with the simpler parsing functions for ease. No harm to leave it on a branch on libtrx for potential future integration? |
Gotcha – I'll open a new PR with |
Done |
That's a separate bug, which happens because Logged as #1463. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Checklist
Description
A loose proposal how to refactor parsing the arguments of the console commands using LostArtefacts/libtrx#19. I refactored a few commands to showcase how this works.
I'm not sure if I like this, honestly, and I'm eager to hear your opinion. It works great for boolean commands, but might make string handling more cumbersome. For instance, the
/play
command accepts a level name, but the new argument parsing requires spaces to be enclosed in quotes (e.g.,/play "great pyramid"
). The problem is, the console does not support all characters, and it's actually impossible to type this.