FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

v9.3.0-mod by stumper66 · Pull Request #522 · CommandAPI/CommandAPI · GitHub

v9.3.0-mod - #522

Closed
stumper66 wants to merge 2 commits into
CommandAPI:dev/devfrom
stumper66:master
Closed

v9.3.0-mod#522
stumper66 wants to merge 2 commits into
CommandAPI:dev/devfrom
stumper66:master

Conversation

Copy link
Copy Markdown

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

Added a new option: skipListValidation
JorelAli changed the base branch from master to dev/dev February 24, 2024 07:10

DerEchtePilz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.


// If the argument's value is in the list of values, include it
List<T> list = new ArrayList<>();
if (skipListValidation) return list;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

*/
public ListArgument<T> buildGreedy() {
return new ListArgument<>(nodeName, delimiter, allowDuplicates, supplier, mapper);
return new ListArgument<>(nodeName, delimiter, allowDuplicates, supplier, mapper, skipListValidation);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This option should not only be used here. If it should be added, the ListTextArgument should also be able to work with this option.

Copy link
Copy Markdown
Collaborator

Just a note that the command you implemented seems to have similar functionality to the FlagsArgument proposed by #483. Development on that is currently in progress on the dev/command-build-rewrite branch 22c2ffa.

Also, dev/argument-exceptions #476 currently provides the API for developers to manually suppress the exceptions thrown when the CommandAPI parses the ListArgument.

slight code change

Copy link
Copy Markdown
Member

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:

/test hello test police

The output I got was [test], the expected result would be [hello, test, police] if the validation is skipped.

DerEchtePilz self-assigned this Feb 26, 2024

Copy link
Copy Markdown
Author

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:

/test hello test police

The output I got was [test], the expected result would be [hello, test, police] if the validation is skipped.

Yeah it's not the most intuitive since the list basically is only good for suggestions.
Afterwards you have to use the rawArgMap and it returns everything in one string.

            .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

DerEchtePilz commented Feb 26, 2024
edited
Loading

Copy link
Copy Markdown
Member

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.
Also, make sure to also update the ListTextArgument constructor so this also can work with this parameter.

Copy link
Copy Markdown
Author

I'll probably just cancel this PR and wait for FlagsArgument proposed by #483 since it likely works better and is more intuitive.

Copy link
Copy Markdown
Member

I'll probably just cancel this PR and wait for FlagsArgument proposed by #483 since it likely works better and is more intuitive.

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.

Copy link
Copy Markdown
Member

Yeah it's not the most intuitive since the list basically is only good for suggestions.

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.
The command sender can then choose which of these items should be used (compare the GIF in the ListArgument's documentation page, there the materials are suggested iirc).
The list that you then can get in the executor contains the chosen values. If not all values are selected, the list that provides suggestions and the list returned by the argument can be very different in size.

Copy link
Copy Markdown
Member

@stumper66
Are you still working on this? If not, it might be better to close this PR since I don't think we want PRs sitting here that are not active anymore.

Copy link
Copy Markdown
Author

Cancelling the PR

stumper66 closed this Apr 11, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL