From 3d1f0c4cc76d01e95e564734f4278151f0c24007 Mon Sep 17 00:00:00 2001 From: Jon Todd Date: Sun, 29 Dec 2013 17:13:54 -0800 Subject: [PATCH] Missing required option doesn't throw This is a proposed fix for: https://github.com/airlift/airline/issues/22 Tests still need to be fixed. --- .../java/io/airlift/airline/ParseResult.java | 22 +++++++++++ .../io/airlift/airline/SingleCommand.java | 37 ++++++++++++++----- .../io/airlift/airline/SingleCommandTest.java | 30 +++++++++++++++ 3 files changed, 80 insertions(+), 9 deletions(-) create mode 100644 src/main/java/io/airlift/airline/ParseResult.java diff --git a/src/main/java/io/airlift/airline/ParseResult.java b/src/main/java/io/airlift/airline/ParseResult.java new file mode 100644 index 000000000..54192707a --- /dev/null +++ b/src/main/java/io/airlift/airline/ParseResult.java @@ -0,0 +1,22 @@ +package io.airlift.airline; + +import com.google.common.collect.ImmutableList; + +import java.util.Collections; +import java.util.List; + +public class ParseResult { + private List errors = Collections.emptyList(); + + public void addError(ParseException error) { + errors.add(error); + } + + public boolean hasErrors() { + return errors.size() != 0; + } + + public List getErrors() { + return ImmutableList.copyOf(errors); + } +} diff --git a/src/main/java/io/airlift/airline/SingleCommand.java b/src/main/java/io/airlift/airline/SingleCommand.java index 8dc5d116d..548168129 100644 --- a/src/main/java/io/airlift/airline/SingleCommand.java +++ b/src/main/java/io/airlift/airline/SingleCommand.java @@ -57,12 +57,31 @@ public C parse(String... args) } public C parse(Iterable args) + { + ParseResult parseResult = new ParseResult(); + C command = parse(parseResult, args); + + // For backward compatibility we fail fast here. Given that exceptions aren't a great way to handle user errors + // since there can be multiple we may consider deprecating this approach. + if (parseResult.hasErrors()) { + throw parseResult.getErrors().get(0); + } + + return command; + } + + public C parse(ParseResult parseResult, String... args) + { + return parse(parseResult, ImmutableList.copyOf(args)); + } + + public C parse(ParseResult parseResult, Iterable args) { checkNotNull(args, "args is null"); - + Parser parser = new Parser(); ParseState state = parser.parseCommand(commandMetadata, args); - validate(state); + validate(state, parseResult); CommandMetadata command = state.getCommand(); @@ -75,35 +94,35 @@ public C parse(Iterable args) ImmutableMap., Object>of(CommandMetadata.class, commandMetadata)); } - private void validate(ParseState state) + private void validate(ParseState state, ParseResult parseResult) { CommandMetadata command = state.getCommand(); if (command == null) { List unparsedInput = state.getUnparsedInput(); if (unparsedInput.isEmpty()) { - throw new ParseCommandMissingException(); + parseResult.addError(new ParseCommandMissingException()); } else { - throw new ParseCommandUnrecognizedException(unparsedInput); + parseResult.addError(new ParseCommandUnrecognizedException(unparsedInput)); } } ArgumentsMetadata arguments = command.getArguments(); if (state.getParsedArguments().isEmpty() && arguments != null && arguments.isRequired()) { - throw new ParseArgumentsMissingException(arguments.getTitle()); + parseResult.addError(new ParseArgumentsMissingException(arguments.getTitle())); } if (!state.getUnparsedInput().isEmpty()) { - throw new ParseArgumentsUnexpectedException(state.getUnparsedInput()); + parseResult.addError(new ParseArgumentsUnexpectedException(state.getUnparsedInput())); } if (state.getLocation() == Context.OPTION) { - throw new ParseOptionMissingValueException(state.getCurrentOption().getTitle()); + parseResult.addError(new ParseOptionMissingValueException(state.getCurrentOption().getTitle())); } for (OptionMetadata option : command.getAllOptions()) { if (option.isRequired() && !state.getParsedOptions().containsKey(option)) { - throw new ParseOptionMissingException(option.getOptions().iterator().next()); + parseResult.addError(new ParseOptionMissingException(option.getOptions().iterator().next())); } } } diff --git a/src/test/java/io/airlift/airline/SingleCommandTest.java b/src/test/java/io/airlift/airline/SingleCommandTest.java index 5439f6802..e1f799b5f 100644 --- a/src/test/java/io/airlift/airline/SingleCommandTest.java +++ b/src/test/java/io/airlift/airline/SingleCommandTest.java @@ -49,6 +49,7 @@ import static com.google.common.base.Predicates.compose; import static com.google.common.base.Predicates.equalTo; +import static com.google.common.base.Predicates.instanceOf; import static com.google.common.collect.Iterables.find; import static io.airlift.airline.SingleCommand.singleCommand; import static org.testng.Assert.assertTrue; @@ -171,6 +172,14 @@ public void multipleUnparsedFail() singleCommand(ArgsMultipleUnparsed.class).parse(); } + @Test + public void multipleUnparsedFailWithResult() + { + ParseResult result = new ParseResult(); + singleCommand(ArgsMultipleUnparsed.class).parse(result); + assertParseResultHasErrorOfType(result, IllegalArgumentException.class); + } + public void privateArgs() { ArgsPrivate args = singleCommand(ArgsPrivate.class).parse("-verbose", "3"); @@ -244,12 +253,27 @@ public void requiredMainParameters() singleCommand(ArgsRequired.class).parse(); } + @Test + public void requiredMainParamertersWithParseResult() { + ParseResult result = new ParseResult(); + singleCommand(ArgsRequired.class).parse(result); + assertParseResultHasErrorOfType(result, ParseException.class); + } + @Test(expectedExceptions = ParseException.class, expectedExceptionsMessageRegExp = ".*option.*missing.*") public void requiredOptions() { singleCommand(OptionsRequired.class).parse(); } + @Test + public void requiredOptionsWithParseResult() + { + ParseResult result = new ParseResult(); + singleCommand(OptionsRequired.class).parse(); + assertParseResultHasErrorOfType(result, ParseException.class); + } + @Test public void ignoresOptionalOptions() { @@ -388,4 +412,10 @@ public static class CommandTest public Boolean interactive = false; } + + private void assertParseResultHasErrorOfType(ParseResult result, Class clazz) { + Assert.assertEquals(result.hasErrors(), true); + String errorMessage = String.format("Expected ParseResult to contain at least one instance of: '%s'", clazz); + Assert.assertNotNull(find(result.getErrors(), instanceOf(IllegalArgumentException.class)), errorMessage); + } }