From 01a294a334bfe71de6a2dbbcfc40fad5a3ebb2b7 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Fri, 18 Dec 2015 22:19:26 +0100 Subject: [PATCH] #305 Adjust behavior of label handling; cleanup - Fix bugs in behavior (wrong labels being shown for help) - Change order of labels and arguments in FoundCommandResult constructors - Move FoundResultStatus enum to its own class - Create test class for HelpProvider --- .../xephi/authme/command/CommandHandler.java | 23 ++--- .../fr/xephi/authme/command/CommandUtils.java | 3 +- .../authme/command/FoundCommandResult.java | 48 ++++------ .../authme/command/FoundResultStatus.java | 16 ++++ .../command/executable/HelpCommand.java | 8 +- .../authme/command/help/HelpProvider.java | 6 +- .../authme/command/CommandHandlerTest.java | 24 +++-- .../authme/command/CommandUtilsTest.java | 89 +++++++++++++++++++ .../authme/command/help/HelpProviderTest.java | 65 ++++++++++++++ 9 files changed, 231 insertions(+), 51 deletions(-) create mode 100644 src/main/java/fr/xephi/authme/command/FoundResultStatus.java create mode 100644 src/test/java/fr/xephi/authme/command/help/HelpProviderTest.java diff --git a/src/main/java/fr/xephi/authme/command/CommandHandler.java b/src/main/java/fr/xephi/authme/command/CommandHandler.java index 1222eeb03..a53a32e1b 100644 --- a/src/main/java/fr/xephi/authme/command/CommandHandler.java +++ b/src/main/java/fr/xephi/authme/command/CommandHandler.java @@ -12,9 +12,9 @@ import java.util.ArrayList; import java.util.List; import java.util.Set; -import static fr.xephi.authme.command.FoundCommandResult.ResultStatus.INCORRECT_ARGUMENTS; -import static fr.xephi.authme.command.FoundCommandResult.ResultStatus.MISSING_BASE_COMMAND; -import static fr.xephi.authme.command.FoundCommandResult.ResultStatus.UNKNOWN_LABEL; +import static fr.xephi.authme.command.FoundResultStatus.INCORRECT_ARGUMENTS; +import static fr.xephi.authme.command.FoundResultStatus.MISSING_BASE_COMMAND; +import static fr.xephi.authme.command.FoundResultStatus.UNKNOWN_LABEL; /** * The AuthMe command handler, responsible for mapping incoming commands to the correct {@link CommandDescription} @@ -121,9 +121,8 @@ public class CommandHandler { // Show a command suggestion if available and the difference isn't too big if (result.getDifference() < SUGGEST_COMMAND_THRESHOLD && result.getCommandDescription() != null) { - sender.sendMessage(ChatColor.YELLOW + "Did you mean " + ChatColor.GOLD + "/" - + result.getCommandDescription() + ChatColor.YELLOW + "?"); - // FIXME: Define a proper string representation of command description + sender.sendMessage(ChatColor.YELLOW + "Did you mean " + ChatColor.GOLD + + CommandUtils.constructCommandPath(result.getCommandDescription()) + ChatColor.YELLOW + "?"); } sender.sendMessage(ChatColor.YELLOW + "Use the command " + ChatColor.GOLD + "/" + result.getLabels().get(0) @@ -175,9 +174,9 @@ public class CommandHandler { List remainingParts = parts.subList(1, parts.size()); CommandDescription childCommand = getSuitableChild(base, remainingParts); if (childCommand != null) { - return new FoundCommandResult(childCommand, parts.subList(2, parts.size()), parts.subList(0, 2)); + return new FoundCommandResult(childCommand, parts.subList(0, 2), parts.subList(2, parts.size())); } else if (hasSuitableArgumentCount(base, remainingParts.size())) { - return new FoundCommandResult(base, parts.subList(1, parts.size()), parts.subList(0, 1)); + return new FoundCommandResult(base, parts.subList(0, 1), parts.subList(1, parts.size())); } return getCommandWithSmallestDifference(base, parts); @@ -200,13 +199,15 @@ public class CommandHandler { // base command may have no children or no child label was present if (closestCommand == null) { - return new FoundCommandResult(null, null, parts, minDifference, UNKNOWN_LABEL); + return new FoundCommandResult(null, parts, null, minDifference, UNKNOWN_LABEL); } - FoundCommandResult.ResultStatus status = (minDifference == 0.0) ? INCORRECT_ARGUMENTS : UNKNOWN_LABEL; + FoundResultStatus status = (minDifference == 0.0) ? INCORRECT_ARGUMENTS : UNKNOWN_LABEL; final int partsSize = parts.size(); List labels = parts.subList(0, Math.min(closestCommand.getParentCount() + 1, partsSize)); - List arguments = parts.subList(Math.min(partsSize - 1, labels.size()), partsSize); + List arguments = (labels.size() == partsSize) + ? new ArrayList() + : parts.subList(labels.size(), partsSize); return new FoundCommandResult(closestCommand, labels, arguments, minDifference, status); } diff --git a/src/main/java/fr/xephi/authme/command/CommandUtils.java b/src/main/java/fr/xephi/authme/command/CommandUtils.java index e0ff117d0..2e4d02a28 100644 --- a/src/main/java/fr/xephi/authme/command/CommandUtils.java +++ b/src/main/java/fr/xephi/authme/command/CommandUtils.java @@ -1,7 +1,6 @@ package fr.xephi.authme.command; import com.google.common.collect.Lists; -import fr.xephi.authme.util.CollectionUtils; import fr.xephi.authme.util.StringUtils; import java.util.ArrayList; @@ -25,7 +24,7 @@ public final class CommandUtils { /** * Provide a textual representation of a list of labels to show it as a command. For example, a list containing - * the items ["authme", "register", "player"] it will return "authme register player". + * the items ["authme", "register", "player"] will return "authme register player". * * @param labels The labels to format * diff --git a/src/main/java/fr/xephi/authme/command/FoundCommandResult.java b/src/main/java/fr/xephi/authme/command/FoundCommandResult.java index 99361d768..76f636336 100644 --- a/src/main/java/fr/xephi/authme/command/FoundCommandResult.java +++ b/src/main/java/fr/xephi/authme/command/FoundCommandResult.java @@ -4,17 +4,17 @@ import java.util.List; /** * Result of a command mapping by {@link CommandHandler}. An object of this class represents a successful mapping - * as well as erroneous ones, as communicated with {@link ResultStatus}. + * as well as erroneous ones, as communicated with {@link FoundResultStatus}. *

- * Fields other than {@link ResultStatus} are available depending, among other factors, on the status: + * Fields other than {@link FoundResultStatus} are available depending, among other factors, on the status: *

    - *
  • {@link ResultStatus#SUCCESS} entails that mapping the input to a command was successful. Therefore, + *
  • {@link FoundResultStatus#SUCCESS} entails that mapping the input to a command was successful. Therefore, * the command description, labels and arguments are set. The difference is 0.0.
    • - *
  • {@link ResultStatus#INCORRECT_ARGUMENTS}: The received parts could be mapped to a command but the argument + *
  • {@link FoundResultStatus#INCORRECT_ARGUMENTS}: The received parts could be mapped to a command but the argument * count doesn't match. Guarantees that the command description field is not null; difference is 0.0
    • - *
  • {@link ResultStatus#UNKNOWN_LABEL}: The labels could not be mapped to a command. The command description may - * be set to the most similar command, or it may be null. Difference is above 0.0.
    • - *
  • {@link ResultStatus#MISSING_BASE_COMMAND} should never occur. All other fields may be null and any further + *
  • {@link FoundResultStatus#UNKNOWN_LABEL}: The labels could not be mapped to a command. The command description + * may be set to the most similar command, or it may be null. Difference is above 0.0.
    • + *
  • {@link FoundResultStatus#MISSING_BASE_COMMAND} should never occur. All other fields may be null and any further * processing of the object should be aborted.
    • *
*/ @@ -24,38 +24,38 @@ public class FoundCommandResult { * The command description instance. */ private final CommandDescription commandDescription; - /** - * The command arguments. - */ - private final List arguments; /** * The labels used to invoke the command. This may be different for the same {@link ExecutableCommand} instance * if multiple labels have been defined, e.g. "/authme register" and "/authme reg". */ private final List labels; + /** + * The command arguments. + */ + private final List arguments; private final double difference; - private final ResultStatus resultStatus; + private final FoundResultStatus resultStatus; /** * Constructor. * * @param commandDescription The command description. + * @param labels The labels used to access the command. * @param arguments The command arguments. - * @param labels The original query reference. */ - public FoundCommandResult(CommandDescription commandDescription, List arguments, List labels, - double difference, ResultStatus resultStatus) { + public FoundCommandResult(CommandDescription commandDescription, List labels, List arguments, + double difference, FoundResultStatus resultStatus) { this.commandDescription = commandDescription; - this.arguments = arguments; this.labels = labels; + this.arguments = arguments; this.difference = difference; this.resultStatus = resultStatus; } - public FoundCommandResult(CommandDescription commandDescription, List arguments, List labels) { - this(commandDescription, arguments, labels, 0.0, ResultStatus.SUCCESS); + public FoundCommandResult(CommandDescription commandDescription, List labels, List arguments) { + this(commandDescription, labels, arguments, 0.0, FoundResultStatus.SUCCESS); } public CommandDescription getCommandDescription() { @@ -74,18 +74,8 @@ public class FoundCommandResult { return difference; } - public ResultStatus getResultStatus() { + public FoundResultStatus getResultStatus() { return resultStatus; } - public enum ResultStatus { - - SUCCESS, - - INCORRECT_ARGUMENTS, - - UNKNOWN_LABEL, - - MISSING_BASE_COMMAND - } } diff --git a/src/main/java/fr/xephi/authme/command/FoundResultStatus.java b/src/main/java/fr/xephi/authme/command/FoundResultStatus.java new file mode 100644 index 000000000..080a3447e --- /dev/null +++ b/src/main/java/fr/xephi/authme/command/FoundResultStatus.java @@ -0,0 +1,16 @@ +package fr.xephi.authme.command; + +/** + * Result status for mapping command parts. See {@link FoundCommandResult} for a detailed description of the states. + */ +public enum FoundResultStatus { + + SUCCESS, + + INCORRECT_ARGUMENTS, + + UNKNOWN_LABEL, + + MISSING_BASE_COMMAND + +} diff --git a/src/main/java/fr/xephi/authme/command/executable/HelpCommand.java b/src/main/java/fr/xephi/authme/command/executable/HelpCommand.java index 0282751aa..95dea98a1 100644 --- a/src/main/java/fr/xephi/authme/command/executable/HelpCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/HelpCommand.java @@ -5,6 +5,7 @@ import fr.xephi.authme.command.CommandHandler; import fr.xephi.authme.command.CommandUtils; import fr.xephi.authme.command.ExecutableCommand; import fr.xephi.authme.command.FoundCommandResult; +import fr.xephi.authme.command.FoundResultStatus; import fr.xephi.authme.command.help.HelpProvider; import fr.xephi.authme.permission.PermissionsManager; import org.bukkit.ChatColor; @@ -12,7 +13,9 @@ import org.bukkit.command.CommandSender; import java.util.List; -import static fr.xephi.authme.command.FoundCommandResult.ResultStatus.*; +import static fr.xephi.authme.command.FoundResultStatus.INCORRECT_ARGUMENTS; +import static fr.xephi.authme.command.FoundResultStatus.MISSING_BASE_COMMAND; +import static fr.xephi.authme.command.FoundResultStatus.UNKNOWN_LABEL; public class HelpCommand extends ExecutableCommand { @@ -27,8 +30,9 @@ public class HelpCommand extends ExecutableCommand { // TODO ljacqu 20151213: This is essentially the same logic as in CommandHandler and we'd like to have the same // messages. Maybe we can have another method in CommandHandler where the end command isn't executed upon // success. - FoundCommandResult.ResultStatus resultStatus = foundCommandResult.getResultStatus(); + FoundResultStatus resultStatus = foundCommandResult.getResultStatus(); if (MISSING_BASE_COMMAND.equals(resultStatus)) { + // FIXME something wrong - this error appears sender.sendMessage(ChatColor.DARK_RED + "Could not get base command"); return; } else if (INCORRECT_ARGUMENTS.equals(resultStatus) || UNKNOWN_LABEL.equals(resultStatus)) { diff --git a/src/main/java/fr/xephi/authme/command/help/HelpProvider.java b/src/main/java/fr/xephi/authme/command/help/HelpProvider.java index 151f3e1bc..a60937cd6 100644 --- a/src/main/java/fr/xephi/authme/command/help/HelpProvider.java +++ b/src/main/java/fr/xephi/authme/command/help/HelpProvider.java @@ -1,5 +1,6 @@ package fr.xephi.authme.command.help; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import fr.xephi.authme.command.CommandArgumentDescription; @@ -216,11 +217,12 @@ public final class HelpProvider { return (flag & options) != 0; } - private static List filterCorrectLabels(CommandDescription command, List labels) { + @VisibleForTesting + protected static List filterCorrectLabels(CommandDescription command, List labels) { List commands = new ArrayList<>(command.getParentCount() + 1); CommandDescription currentCommand = command; while (currentCommand != null) { - commands.add(command); + commands.add(currentCommand); currentCommand = currentCommand.getParent(); } commands = Lists.reverse(commands); diff --git a/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java b/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java index 31810210f..2e46a30ab 100644 --- a/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java +++ b/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java @@ -53,7 +53,7 @@ public class CommandHandlerTest { // then assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("login", "authme"))); - assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.SUCCESS)); + assertThat(result.getResultStatus(), equalTo(FoundResultStatus.SUCCESS)); assertThat(result.getArguments(), contains("test1")); assertThat(result.getDifference(), equalTo(0.0)); } @@ -68,7 +68,7 @@ public class CommandHandlerTest { // then assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme"))); - assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.SUCCESS)); + assertThat(result.getResultStatus(), equalTo(FoundResultStatus.SUCCESS)); assertThat(result.getArguments(), contains("arg1", "arg2")); assertThat(result.getDifference(), equalTo(0.0)); } @@ -83,7 +83,21 @@ public class CommandHandlerTest { // then assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme"))); - assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.INCORRECT_ARGUMENTS)); + assertThat(result.getResultStatus(), equalTo(FoundResultStatus.INCORRECT_ARGUMENTS)); + assertThat(result.getDifference(), equalTo(0.0)); + } + + @Test + public void shouldRejectCommandWithTooFewArguments() { + // given + List parts = Arrays.asList("authme", "Reg"); + + // when + FoundCommandResult result = handler.mapPartsToCommand(parts); + + // then + assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme"))); + assertThat(result.getResultStatus(), equalTo(FoundResultStatus.INCORRECT_ARGUMENTS)); assertThat(result.getDifference(), equalTo(0.0)); } @@ -97,7 +111,7 @@ public class CommandHandlerTest { // then assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme"))); - assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.UNKNOWN_LABEL)); + assertThat(result.getResultStatus(), equalTo(FoundResultStatus.UNKNOWN_LABEL)); assertThat(result.getDifference() < 0.75, equalTo(true)); } @@ -112,7 +126,7 @@ public class CommandHandlerTest { // then assertThat(result.getCommandDescription(), not(nullValue())); - assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.UNKNOWN_LABEL)); + assertThat(result.getResultStatus(), equalTo(FoundResultStatus.UNKNOWN_LABEL)); assertThat(result.getDifference() > 0.75, equalTo(true)); } diff --git a/src/test/java/fr/xephi/authme/command/CommandUtilsTest.java b/src/test/java/fr/xephi/authme/command/CommandUtilsTest.java index 1d7baaab4..0f4dff18d 100644 --- a/src/test/java/fr/xephi/authme/command/CommandUtilsTest.java +++ b/src/test/java/fr/xephi/authme/command/CommandUtilsTest.java @@ -8,6 +8,7 @@ import java.util.List; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.mock; /** * Test for {@link CommandUtils}. @@ -37,4 +38,92 @@ public class CommandUtilsTest { // then assertThat(str, equalTo("")); } + + @Test + public void shouldPrintLabels() { + // given + List labels = Arrays.asList("authme", "help", "reload"); + + // when + String result = CommandUtils.labelsToString(labels); + + // then + assertThat(result, equalTo("authme help reload")); + } + + @Test + public void shouldReturnCommandPath() { + // given + CommandDescription base = CommandDescription.builder() + .labels("authme", "auth") + .description("Base") + .detailedDescription("Test base command.") + .executableCommand(mock(ExecutableCommand.class)) + .build(); + CommandDescription command = CommandDescription.builder() + .parent(base) + .labels("help", "h", "?") + .description("Child") + .detailedDescription("Test child command.") + .executableCommand(mock(ExecutableCommand.class)) + .build(); + + // when + String commandPath = CommandUtils.constructCommandPath(command); + + // then + assertThat(commandPath, equalTo("/authme help")); + } + + + // ------ + // min / max arguments + // ------ + @Test + public void shouldComputeMinAndMaxOnEmptyCommand() { + // given + CommandDescription command = getBuilderForArgsTest().build(); + + // when / then + checkArgumentCount(command, 0, 0); + } + + @Test + public void shouldComputeMinAndMaxOnCommandWithMandatoryArgs() { + // given + CommandDescription command = getBuilderForArgsTest() + .withArgument("Test", "Arg description", false) + .withArgument("Test22", "Arg description 2", false) + .build(); + + // when / then + checkArgumentCount(command, 2, 2); + } + + @Test + public void shouldComputeMinAndMaxOnCommandIncludingOptionalArgs() { + // given + CommandDescription command = getBuilderForArgsTest() + .withArgument("arg1", "Arg description", false) + .withArgument("arg2", "Arg description 2", true) + .withArgument("arg3", "Arg description 3", true) + .build(); + + // when / then + checkArgumentCount(command, 1, 3); + } + + + private static void checkArgumentCount(CommandDescription command, int expectedMin, int expectedMax) { + assertThat(CommandUtils.getMinNumberOfArguments(command), equalTo(expectedMin)); + assertThat(CommandUtils.getMaxNumberOfArguments(command), equalTo(expectedMax)); + } + + private static CommandDescription.CommandBuilder getBuilderForArgsTest() { + return CommandDescription.builder() + .labels("authme", "auth") + .description("Base") + .detailedDescription("Test base command.") + .executableCommand(mock(ExecutableCommand.class)); + } } diff --git a/src/test/java/fr/xephi/authme/command/help/HelpProviderTest.java b/src/test/java/fr/xephi/authme/command/help/HelpProviderTest.java new file mode 100644 index 000000000..dbc62dc85 --- /dev/null +++ b/src/test/java/fr/xephi/authme/command/help/HelpProviderTest.java @@ -0,0 +1,65 @@ +package fr.xephi.authme.command.help; + +import fr.xephi.authme.command.CommandDescription; +import fr.xephi.authme.command.ExecutableCommand; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.util.Arrays; +import java.util.List; + +import static org.hamcrest.Matchers.contains; +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.mock; + +/** + * Test for {@link HelpProvider}. + */ +public class HelpProviderTest { + + private static CommandDescription parent; + private static CommandDescription child; + + @BeforeClass + public static void setUpCommands() { + parent = CommandDescription.builder() + .executableCommand(mock(ExecutableCommand.class)) + .labels("base", "b") + .description("Parent") + .detailedDescription("Test base command.") + .build(); + child = CommandDescription.builder() + .executableCommand(mock(ExecutableCommand.class)) + .parent(parent) + .labels("child", "c") + .description("Child") + .detailedDescription("Child test command.") + .withArgument("Argument", "The argument", false) + .build(); + } + + @Test + public void shouldRetainCorrectLabels() { + // given + List labels = Arrays.asList("b", "child"); + + // when + List result = HelpProvider.filterCorrectLabels(child, labels); + + // then + assertThat(result, contains("b", "child")); + } + + @Test + public void shouldReplaceIncorrectLabels() { + // given + List labels = Arrays.asList("base", "wrong"); + + // when + List result = HelpProvider.filterCorrectLabels(child, labels); + + // then + assertThat(result, contains("base", "child")); + } + +}