-
-
Notifications
You must be signed in to change notification settings - Fork 2
Add client-heavy multi command system (and experimental rewrite) #18
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
base: main
Are you sure you want to change the base?
Conversation
|
Also, can someone test if you are able to run namespaces with capital letters inside? I forgot to check if that still works. |
|
It does work without the string.lower, but we should play safe and use a string.lower when doing namespace lookup anyway. |
all network callbacks are called with a Player
- allows namespaces to run their own callbacks - mod commands can be implemented easier - this is NOT wired to anything yet because it is a prototype - client can call forward(...) to invoke server code safely - forward is NOT implicitly called in client_registry.luau because i am scared it can cause bugs - we need a more ergonomic way to port default & get because help_callback feels weird to use
…(legacy logic) - should still not used in ANY code yet - added forward in help_callback; the rationale for this is that maybe some namespaces have custom server logic which can influence help results? i was mainly thinking of dynamically updated mod command help texts/ranking (this is a nitpick / extra feature) - we should figure out how to get nice, ergonomic Result types because error handling in Luau is not pretty and currently we have to do alot of casts - serverDefineSimple is a shim to support literally just copy pasting old define code blocks and it should help them work; we should port this old code to a cleaner, newer interface but i don't have any ideas yet
| help_callback = function(): impl.HelpResults? | ||
| return { | ||
| c = table.freeze({ | ||
| aliases = { "run/" }, |
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.
"run" and "c" (execute) are different commands
|
|
||
| help_callback = function(): impl.HelpResults? | ||
| return { | ||
| c = table.freeze({ |
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.
Technically the real command name is "execute" with "c" being an alias for it
|
|
||
| return { | ||
| table.freeze({ | ||
| prefixes = { "c", "run" }, |
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.
The default namespace isn't supposed to have prefixes? Prefixes are what you add before your command string to tell the parser to look at commands from that prefix (so like the "get" or "g" in get/respawn)
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.
I don't exactly understand the reason for defining (not the define() function, that should be called implement() tbh) commands in code rather than in toml files like before.
It also doesn't look like all commands have gotten implemented yet?
I am ok with parsing multiple commands on client which then tells the server which ones to run.
But my solution to this was to add an actual "get" command that splits the 1st argument it receives by spaces and then run each one of those as it's own command from the "get" namespace, it then receiving the context.Data object from each of those commands after they run and then passing all of those to the server version of the "get" command which can then pass the client context data to the respective command it runs.
In short moving the namespace parsing code into their own commands and only executing commands in the default namespace from the chat / console. (This comes with the benefit that the "get" command, or in the future, a "sb" / "mod" command can check permissions before it runs any actual commands).
|
I haven't implemented all commands yet because I wanted to see we wanted to keep going with this approach. The "default" namespace was a test snippet, because I wanted to see if the API was ergonomic enough and it partially serves as an example namespace. I don't think using TOML is a bad idea, and the define() function was to see if it was possible to quickly port old code. The main reason I moved some TOML data was to prototype a better API surface. But yeah, we should be using TOML for description, aliases, name, arguments (?), etc. |
We can rely on the client to provide multi command handling.
This is still missing g/no. support (which might be hard to add?).