Use StringArgumentType#string()
for ServerCommand
server argument
#1363
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #1362
Currently, the
/server
command usesStringArgumentType#word
for its argument. This argument only accepts a limited set of characters, as defined by Brigadier'sStringReader#isAllowedInUnquotedString
method. It is possible to give a backend server a name that contains characters outside of this set, making that server unusable with the/server
command.This PR changes the
/server
command to use theStringArgumentType#string
argument type. This argument accepts unquoted strings likeword
, so previously valid commands are still valid. However, any characters can now be entered, as long as the argument is surrounded by"
.The suggestion provider for the argument was updated to suggest quotes around server names that need to be quoted. If the user places a quote at the start of the argument, then all the server names are suggested surrounded by quotes. If the user types a character other than a quote, names that require quotes are not suggested.
I wasn't sure if I should write automated tests for this new code. I didn't see any tests for the functionality of builtin Velocity commands, so I decided not to add any suggestions tests in this PR.
I also noticed that the
/glist
command and the/send
command also useStringArgumentType#word
to accept a target server name. Should those commands be updated to useStringArgumentType#string
as well? Also, would it be a good idea to somehow share the code for the server name suggestions provider, rather than duplicating (mostly,glist
also suggests"all"
) the same logic across these classes?As an alternative solution, I believe
StringArgumentType#greedy
would be appropriate here. This is the last argument in the command, and thestring
argument type allows the user to input arbitrary strings anyway. On the user side, using a greedy string would mean they wouldn't need to put quotes around the server name. On the code side, the suggestions provider wouldn't have to worry about handling server names that are not valid unquoted strings. However, #1362 (comment) alluded to the idea that a greedy string here may be problematic ¯\(ツ)/¯.As another alternative solution, I suppose Velocity could prohibit server names that are not unquoted strings. #1362 (comment) suggests that this is a niche setup that often causes other issues anyway. However, I imagine this isn't really an acceptable solution for users whose language doesn't exclusively use the letters a-z.