-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
v9.3.0-mod #522
v9.3.0-mod #522
Conversation
Added a new option: skipListValidation
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.
A few things I noticed that I believe should be addressed.
Otherwise I guess I like this concept. It is a fairly niche feature but I don't see a reason why it shouldn't be added.
@@ -132,6 +134,7 @@ public <CommandSourceStack> List<T> parseArgument(CommandContext<CommandSourceSt | |||
|
|||
// If the argument's value is in the list of values, include it | |||
List<T> list = new ArrayList<>(); | |||
if (skipListValidation) return list; |
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 am unsure about this check being here. I mean, despite not checking if only valid values were given, you probably still would want access to the given list in the argument.
I'd suggest using the boolean to disable the exceptions from being thrown instead of using it to return an empty list.
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 was originally doing it this way but then discovered that the list was always empty so after some testing I decided to just have it return early instead.
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.
In this case, an alternate way of parsing this argument would have to be created. The list returned should contain the values provided by the user. I am not willing to say that users will have to keep track of their own list in order to validate results or something because returning an empty list is basically not acceptable.
@@ -161,7 +173,7 @@ private ListArgumentBuilderFinished(Function<T, IStringTooltip> mapper) { | |||
* @return a {@link ListArgument} | |||
*/ | |||
public ListArgument<T> buildGreedy() { | |||
return new ListArgument<>(nodeName, delimiter, allowDuplicates, supplier, mapper); | |||
return new ListArgument<>(nodeName, delimiter, allowDuplicates, supplier, mapper, skipListValidation); |
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.
This option should not only be used here. If it should be added, the ListTextArgument
should also be able to work with this option.
Just a note that the command you implemented seems to have similar functionality to the Also, |
slight code change
This latest iteration doesn't work. I tested this command: new CommandAPICommand("test")
.withArguments(new ListArgumentBuilder<String>("list")
.skipListValidation(true)
.withList("test")
.withStringMapper()
.buildGreedy()
)
.executesPlayer(info -> {
List<String> list = (List<String>) info.args().get("list");
info.sender().sendMessage(Component.text().content(list.toString()).build());
})
.register(); I then executed this command:
The output I got was |
Yeah it's not the most intuitive since the list basically is only good for suggestions. .executes(CommandExecutor { sender, args -> processCmd(sender, args) })
# ...
.withOptionalArguments(
ListArgumentBuilder<String>("values")
.skipListValidation(true)
.withList { info -> buildTabSuggestions(info) }
.withStringMapper()
.buildGreedy()
val values = args.rawArgsMap["values"]
val args = splitStringWithQuotes(values) # split into array and treat quotes as one element Here's how I used it https://github.com/ArcanePlugins/LevelledMobs/blob/4.0.0-dev/levelledmobs-plugin/src/main/kotlin/io/github/arcaneplugins/levelledmobs/commands/subcommands/SummonSubcommand.kt#L56 |
Nope, that's not how it's going to work when merged; the List returned has to contain every relevant value. As I said, it would be best to have a separate parsing method when list validation is skipped. |
I'll probably just cancel this PR and wait for |
I mean, do what you want to do, but again, this, despite being a really niche feature, can be useful. It just needs a bit more attention since there are expectations when using the list. |
Also, not true. The list, if validations are not skipped, provides the player or any other command executor with a list of options. The items in the suggestion list are there to tell the user the valid options. |
@stumper66 |
Cancelling the PR |
Added a new option: skipListValidation
This allows me to use ListArgumentBuilder for complex tab suggestions and simply passing all input so I can do my own validation.
This allowed me to create this command: https://youtu.be/q03_paw9Vzk