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

SlashCommands: report an error if a parameter is expected but missing #829

Open
dijkstracula opened this issue Oct 21, 2023 · 3 comments
Open

Comments

@dijkstracula
Copy link

dijkstracula commented Oct 21, 2023

Hello Azure team,

I'm currently attending the Roguelike Celebration and am enjoying exploring the virtual space. I noticed that if I fail to give an parameter to a slash command that expects one, such as /look, I get an error that reads, "Your command /look is not a registered slash command!". However, the problem is not that it's not registered, because of course it is, but rather that an argument was expected. I would expect an error closer to the form, "Your command /look requires an argument" or some such thing.

I'm not a TypeScript person, but, It seems like SlashCommands::matchingSlashCommand could return a union literal type of the form SlashCommand | "ParamExpected" | "UnregisteredCommand", which the caller in reducer.ts would match on. If you agree that this is a reasonable approach, I'm happy to put out a patch.

Cheers,
Nathan

@dijkstracula
Copy link
Author

Realizing that the inverse is true, too: if I provide a parameter to a command that should not consume one, it's silently discarded. (This seems reasonable but I suppose one might want to warn on this case, too?)

@dijkstracula
Copy link
Author

dijkstracula commented Oct 21, 2023

One more variation on this issue: I noticed that parsed commands tend to assume exactly one space between a slash command and its argument(s). This happens both in the frontend (e.g. const commandStr = /^(\/.+?) (.+)/.exec(trimmedMessage)) and the backend (e.g. const moveMatch = /^\/(go|move) (.+)/.exec(message)).

So, If I am currently in the ice rink, /go theater, as one expects, takes me to the theater; however, /go theater does not, since I suppose there is no room named theater (e.g. three spaces followed by the word "theater"). (Notably, the error I get is "Hmm... theater isn't a match to any rooms.", suggesting the backend does strip leading whitespace at some point but I can't see exactly how in moveToRoom.ts)

Seems like either the regexen should eat arbitrary whitespace, or favour split() (though the latter may prove frictive with arguments where spaces are expected, like room names)

@dijkstracula
Copy link
Author

One additional tiny thing: Because of the following code path

    } else if (beginsWithSlash) {
      sendChatMessage(messageId, trimmedMessage)
    } 

I seem to be able to pass through / metacharacters. Seems like this is another case where presenting an error to the user might be warranted? I don't know what you would all prefer in terms of usability, though.

image

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

No branches or pull requests

1 participant