Index: /trunk/src/org/openstreetmap/josm/tools/OptionParser.java =================================================================== --- /trunk/src/org/openstreetmap/josm/tools/OptionParser.java (revision 14639) +++ /trunk/src/org/openstreetmap/josm/tools/OptionParser.java (revision 14640) @@ -42,8 +42,8 @@ public OptionParser addShortAlias(String optionName, String shortName) { if (!shortName.matches("\\w")) { - throw new IllegalArgumentException("Short name " + shortName + " must be one character"); + throw new IllegalArgumentException("Short name '" + shortName + "' must be one character"); } if (availableOptions.containsKey("-" + shortName)) { - throw new IllegalArgumentException("Short name " + shortName + " is already used"); + throw new IllegalArgumentException("Short name '" + shortName + "' is already used"); } AvailableOption longDefinition = availableOptions.get("--" + optionName); @@ -70,8 +70,8 @@ private void checkOptionName(String optionName) { if (!optionName.matches("\\w([\\w-]*\\w)?")) { - throw new IllegalArgumentException("Illegal option name: " + optionName); + throw new IllegalArgumentException("Illegal option name: '" + optionName + "'"); } if (availableOptions.containsKey("--" + optionName)) { - throw new IllegalArgumentException("The option --" + optionName + " is already registered"); + throw new IllegalArgumentException("The option '--" + optionName + "' is already registered"); } } @@ -164,5 +164,5 @@ } else { if (toHandle.isEmpty() || "--".equals(toHandle.getFirst())) { - throw new OptionParseException(tr("{0}: option ''{1}'' requires an argument", program)); + throw new OptionParseException(tr("{0}: option ''{1}'' requires an argument", program, optionName)); } parameter = toHandle.removeFirst(); @@ -179,10 +179,15 @@ availableOptions.values().stream().distinct().forEach(def -> { long count = options.stream().filter(p -> def.equals(p.option)).count(); + String optionName = availableOptions.entrySet().stream() + .filter(entry -> def.equals(entry.getValue())) + .map(Entry::getKey) + .findFirst() + .orElse("?"); if (count < def.getRequiredCount().min) { // min may be 0 or 1 at the moment - throw new OptionParseException(tr("{0}: option ''{1}'' is required")); + throw new OptionParseException(tr("{0}: option ''{1}'' is required", program, optionName)); } else if (count > def.getRequiredCount().max) { // max may be 1 or MAX_INT at the moment - throw new OptionParseException(tr("{0}: option ''{1}'' may not appear multiple times")); + throw new OptionParseException(tr("{0}: option ''{1}'' may not appear multiple times", program, optionName)); } }); @@ -222,5 +227,5 @@ return alternatives.get(0); } else if (alternatives.size() > 1) { - throw new OptionParseException(tr("{0}: option ''{1}'' is ambiguous", program)); + throw new OptionParseException(tr("{0}: option ''{1}'' is ambiguous", program, optionName)); } } Index: /trunk/test/unit/org/openstreetmap/josm/tools/OptionParserTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/tools/OptionParserTest.java (revision 14639) +++ /trunk/test/unit/org/openstreetmap/josm/tools/OptionParserTest.java (revision 14640) @@ -12,5 +12,9 @@ import java.util.concurrent.atomic.AtomicReference; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.openstreetmap.josm.testutils.JOSMTestRules; import org.openstreetmap.josm.tools.OptionParser.OptionCount; import org.openstreetmap.josm.tools.OptionParser.OptionParseException; @@ -22,23 +26,44 @@ public class OptionParserTest { + /** + * Rule used for tests throwing exceptions. + */ + @Rule + public ExpectedException thrown = ExpectedException.none(); + + /** + * Setup test. + */ + @Rule + @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD") + public JOSMTestRules test = new JOSMTestRules().i18n(); + // A reason for moving to jupiter... - @Test(expected = OptionParseException.class) + @Test public void testEmptyParserRejectsLongopt() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: unrecognized option '--long'"); new OptionParser("test").parseOptions(Arrays.asList("--long")); } - @Test(expected = OptionParseException.class) + @Test public void testEmptyParserRejectsShortopt() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: unrecognized option '-s'"); new OptionParser("test").parseOptions(Arrays.asList("-s")); } - @Test(expected = OptionParseException.class) + @Test public void testParserRejectsWrongShortopt() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: unrecognized option '-s'"); new OptionParser("test").addFlagParameter("test", this::nop).addShortAlias("test", "t") .parseOptions(Arrays.asList("-s")); } - @Test(expected = OptionParseException.class) + @Test public void testParserRejectsWrongLongopt() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: unrecognized option '--wrong'"); new OptionParser("test").addFlagParameter("test", this::nop).parseOptions(Arrays.asList("--wrong")); } @@ -54,6 +79,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testParserOptionFailsIfMissing() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: unrecognized option '--test2'"); AtomicReference argFound = new AtomicReference<>(); OptionParser parser = new OptionParser("test") @@ -63,6 +90,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testParserOptionFailsIfMissingArgument() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: unrecognized option '--test2'"); AtomicReference argFound = new AtomicReference<>(); OptionParser parser = new OptionParser("test") @@ -72,6 +101,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testParserOptionFailsIfMissing2() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '--test' is required"); AtomicReference argFound = new AtomicReference<>(); OptionParser parser = new OptionParser("test") @@ -81,6 +112,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testParserOptionFailsIfTwice() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '--test' may not appear multiple times"); AtomicReference argFound = new AtomicReference<>(); OptionParser parser = new OptionParser("test") @@ -90,6 +123,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testParserOptionFailsIfTwiceForAlias() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '-t' may not appear multiple times"); AtomicReference argFound = new AtomicReference<>(); OptionParser parser = new OptionParser("test") @@ -100,6 +135,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testOptionalOptionFailsIfTwice() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '--test' may not appear multiple times"); OptionParser parser = new OptionParser("test") .addFlagParameter("test", this::nop); @@ -107,6 +144,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testOptionalOptionFailsIfTwiceForAlias() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '-t' may not appear multiple times"); OptionParser parser = new OptionParser("test") .addFlagParameter("test", this::nop) @@ -115,6 +154,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testOptionalOptionFailsIfTwiceForAlias2() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '-t' may not appear multiple times"); OptionParser parser = new OptionParser("test") .addFlagParameter("test", this::nop) @@ -145,6 +186,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testLongArgumentsMissingOption() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '--test' requires an argument"); OptionParser parser = new OptionParser("test") .addArgumentParameter("test", OptionCount.REQUIRED, this::nop); @@ -153,6 +196,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testLongArgumentsMissingOption2() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '--test' requires an argument"); OptionParser parser = new OptionParser("test") .addArgumentParameter("test", OptionCount.REQUIRED, this::nop); @@ -161,6 +206,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testShortArgumentsMissingOption() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '-t' requires an argument"); OptionParser parser = new OptionParser("test") .addArgumentParameter("test", OptionCount.REQUIRED, this::nop) @@ -170,6 +217,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testShortArgumentsMissingOption2() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '-t' requires an argument"); OptionParser parser = new OptionParser("test") .addArgumentParameter("test", OptionCount.REQUIRED, this::nop) @@ -179,6 +228,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testLongFlagHasOption() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '--test' does not allow an argument"); OptionParser parser = new OptionParser("test") .addFlagParameter("test", this::nop); @@ -187,6 +238,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testShortFlagHasOption() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '-t' does not allow an argument"); OptionParser parser = new OptionParser("test") .addFlagParameter("test", this::nop) @@ -251,6 +304,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testAmbiguousAlternatives() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: option '--fl' is ambiguous"); AtomicReference argFound = new AtomicReference<>(); AtomicBoolean usedFlag = new AtomicBoolean(); @@ -265,6 +320,8 @@ } - @Test(expected = OptionParseException.class) + @Test public void testMultipleShort() { + thrown.expect(OptionParseException.class); + thrown.expectMessage("test: unrecognized option '-ft'"); AtomicReference argFound = new AtomicReference<>(); AtomicBoolean usedFlag = new AtomicBoolean(); @@ -300,52 +357,72 @@ } - @Test(expected = IllegalArgumentException.class) + @Test public void testIllegalOptionName() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Illegal option name: ''"); new OptionParser("test").addFlagParameter("", this::nop); } - @Test(expected = IllegalArgumentException.class) + @Test public void testIllegalOptionName2() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Illegal option name: '-'"); new OptionParser("test").addFlagParameter("-", this::nop); } - @Test(expected = IllegalArgumentException.class) + @Test public void testIllegalOptionName3() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Illegal option name: '-test'"); new OptionParser("test").addFlagParameter("-test", this::nop); } - @Test(expected = IllegalArgumentException.class) + @Test public void testIllegalOptionName4() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Illegal option name: '$'"); new OptionParser("test").addFlagParameter("$", this::nop); } - @Test(expected = IllegalArgumentException.class) + @Test public void testDuplicateOptionName() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("The option '--test' is already registered"); new OptionParser("test").addFlagParameter("test", this::nop).addFlagParameter("test", this::nop); } - @Test(expected = IllegalArgumentException.class) + @Test public void testDuplicateOptionName2() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("The option '--test' is already registered"); new OptionParser("test").addFlagParameter("test", this::nop) .addArgumentParameter("test", OptionCount.OPTIONAL, this::nop); } - @Test(expected = IllegalArgumentException.class) + @Test public void testInvalidShortAlias() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Short name '$' must be one character"); new OptionParser("test").addFlagParameter("test", this::nop).addShortAlias("test", "$"); } - @Test(expected = IllegalArgumentException.class) + @Test public void testInvalidShortAlias2() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Short name '' must be one character"); new OptionParser("test").addFlagParameter("test", this::nop).addShortAlias("test", ""); } - @Test(expected = IllegalArgumentException.class) + @Test public void testInvalidShortAlias3() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Short name 'xx' must be one character"); new OptionParser("test").addFlagParameter("test", this::nop).addShortAlias("test", "xx"); } - @Test(expected = IllegalArgumentException.class) + @Test public void testDuplicateShortAlias() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Short name 't' is already used"); new OptionParser("test").addFlagParameter("test", this::nop) .addFlagParameter("test2", this::nop) @@ -354,6 +431,9 @@ } - @Test(expected = IllegalArgumentException.class) + @Test public void testInvalidShortNoLong() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("No long definition for test2 was defined. " + + "Define the long definition first before creating a short definition for it."); new OptionParser("test").addFlagParameter("test", this::nop).addShortAlias("test2", "t"); }