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

Make sure arguments are preserved after redirects #142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

willkroboth
Copy link

This PR resolves #137 and remakes #138.


Summary:

When suggesting arguments after a redirect, calling CommandContext#getArgument on the given context can only access arguments declared before the redirect.

When executing a command after a redirect, calling CommandContext#getArgument on the given context can only access arguments declared after the redirect.

These two situations seem to handle arguments differently. This seems like it might be a bug, so I created #137 focusing on the problem with executing commands. It's possible this is intended behavior, but I couldn't find any documentation stating this either way. No one has confirmed or denied #137 yet, so I'm assuming this is a bug.


Without this PR, this example test for command dispatch fails:

public class CommandDispatcherTest {
    // Setup stuff
    @Test
    public void testRedirectPreservesPreviousArguments() throws CommandSyntaxException {
        final LiteralCommandNode<Object> ending = literal("ending")
                .executes(context -> context.getArgument("number", int.class)).build();
        final ArgumentCommandNode<Object, Integer> lowNumber = argument("number", integer(1, 10))
                .then(ending).build();
        final ArgumentCommandNode<Object, Integer> highNumber = argument("number", integer(11, 20))
                .redirect(lowNumber).build();
        subject.register(literal("base")
                .then(literal("low").then(lowNumber))
                .then(literal("high").then(highNumber)));

        assertThat(subject.execute("base low 5 ending", source), is(5));
        assertThat(subject.execute("base high 15 ending", source), is(15));
    }
}

Specifically, the exception java.lang.IllegalArgumentException: No such argument 'number' exists on this command is thrown when evaluating the second assertion. When parsing "base high 15 ending", a CommandContext something like the following is created:

CommandContext {
    range: "base low 15"
    arguments: {"number"->15}
    child: CommandContext {
        range: "ending"
        arguments: {}
    }
}

When the command is executed, it uses the last child. This child does not know about the number argument, hence the IllegalArgumentException.

This PR makes it so that when parsing nodes redirects, the arguments from the parent CommandContext are copied to the child. That means parsing "base high 15 ending" gives the following context:

CommandContext {
    range: "base low 15"
    arguments: {"number"->15}
    child: CommandContext {
        range: "ending"
        arguments: {"number"->15}
    }
}

So, when the last child is used to execute the command, it now knows about the number argument, and the executor works as expected.


I also thought it was weird that this example test for suggestions failed:

public class CommandSuggestionsTest {
    // Setup stuff
    @Test
    public void getCompletionSuggestions_redirectPreservesArguments() throws Exception {
        subject.register(literal("command")
                .then(
                        argument("first", integer())
                                .then(
                                        argument("second", integer())
                                                .suggests((context, builder) -> {
                                                    builder.suggest(String.valueOf(context.getArgument("first", int.class) + 1));
                                                    return builder.buildFuture();
                                                })
                                )
                ));
        subject.register(literal("redirect").redirect(subject.getRoot()));

        testSuggestions("command 1 ", 10, StringRange.at(10), "2");
        testSuggestions("redirect command 1 ", 19, StringRange.at(19), "2");
        testSuggestions("redirect redirect command 1 ", 28, StringRange.at(28), "2");
    }
}

When creating suggestions for "command <first> <second>", <second> is supposed to get the suggestion of <first>+1. This works if you just input "command <first> ", so the first test passes. However, after a redirect, it doesn't work. In the second test, I'd expect that "redirect command 1 " suggests "2". However, the exception java.lang.IllegalArgumentException: No such argument 'number' exists on this command is thrown instead. The CommandContext for this would look something like this:

CommandContext {
    range: "redirect"
    arguments: {}
    child: CommandContext {
        range: "command 1 "
        arguments: {"first"->1}
    }
}

When creating the suggestions, this whole context is used. So, just calling CommandContext#getArgument, the first argument can not be found since the parent context does not have any arguments.

I thought this might be a bug. It makes more sense that the context representing the range that is currently being suggested should be used to get the suggestions. Here, that would mean using the child CommandContext, which does have the first argument.

However, this modified test does pass:

public class CommandSuggestionsTest {
    // Setup stuff
    @Test
    public void getCompletionSuggestions_redirectPreservesArguments() throws Exception {
        subject.register(literal("command")
                .then(
                        argument("first", integer())
                                .then(
                                        argument("second", integer())
                                                .suggests((context, builder) -> {
                                                    builder.suggest(String.valueOf(context.getLastChild().getArgument("first", int.class) + 1));
                                                    return builder.buildFuture();
                                                })
                                )
                ));
        subject.register(literal("redirect").redirect(subject.getRoot()));

        testSuggestions("command 1 ", 10, StringRange.at(10), "2");
        testSuggestions("redirect command 1 ", 19, StringRange.at(19), "2");
        testSuggestions("redirect redirect command 1 ", 28, StringRange.at(28), "2");
    }
}

Instead of using the given CommandContext directly, the SuggestionsProvider calls CommandContext#getLastChild(). I think this makes sense. The part of the command being suggested should always be the last part of the command, and hence the last child of the CommandContext. This makes my test pass, so I guess it's okay?

Not sure. If my assumption that the suggestions are only created at the end of the command is wrong, please correct me. In that case, I think it should be easy enough to change the suggestions code so that it choose the child CommandContext for the node that is currently being suggested.

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.

Arguments in a CommandContext disappear when a node redirects
1 participant