From 8bd62d045931bea94ad7767dc7a9eb975ee7d256 Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Tue, 16 Aug 2016 23:18:08 -0400 Subject: [PATCH] Various fixes for usages derived from the `@arg` annotation. Enforce if both 'flag' and 'name' are given in the `@arg` annotation, the length of 'flag' is less than or equal to the length of 'name'. It is ok if the length of 'name' derived from the constructor-parameter/field is less than the length of 'flag', and in this cause we'll print 'flag' second. Examples: 1. `@arg(flag="abc", name="a") val a: T` should be illegal 2. `@arg(flag="abc") val a: T` will have usage "-a T, --abc=T" Previously it would not check and print the following usages for 1-2 above: 1. "-abc T, --a=T". Note that specifying either "-abc T" or "--a T" wouldn't work. 2. "-abc T, -a=T". Note that specifying "-abc T" wouldn't work. Format a single-character flag correctly if specified in name or the variable/field is a single-character. - `@arg(name="a") val abc: T` will have usage "-a T". - `@arg val a: T` will have usage "-a T". Add a regression test for the 'name' field being camel case: - `@arg(name="aB") val a: T` will have usage "--aB=T". Fix the usage for multi-character flag arguments. For example, if `@arg(flag="abc") val abcd: T` then the usage will be "--abc=T, --abcd=T"; it was previously "-abc T, --abcd=T". Updated the documentation for ArgAnnotation for all the above. --- .../java/dagr/sopt/cmdline/ArgAnnotation.java | 9 +- .../ClpArgumentDefinitionPrinting.scala | 22 +++- .../sopt/cmdline/ClpReflectiveBuilder.scala | 16 ++- .../CommandLineProgramParserTest.scala | 121 +++++++++++++++++- 4 files changed, 153 insertions(+), 15 deletions(-) diff --git a/sopt/src/main/java/dagr/sopt/cmdline/ArgAnnotation.java b/sopt/src/main/java/dagr/sopt/cmdline/ArgAnnotation.java index 5402848a..e9ce7f7c 100644 --- a/sopt/src/main/java/dagr/sopt/cmdline/ArgAnnotation.java +++ b/sopt/src/main/java/dagr/sopt/cmdline/ArgAnnotation.java @@ -51,9 +51,12 @@ String name() default ""; /** - * Specified short name of the command. Short names should be prefixed - * with a single dash. ArgAnnotation values can directly abut single-char - * short names or be separated from them by a space. + * Specified short name of the command. Single value short names should be + * prefixed on the command-line with a single dash. Multi-character short-names + * will be treated like {@link #name()}. ArgAnnotation values can directly abut + * single-char short names or be separated from them by a space. The short name + * should always be shorter than {@link #name()} if the latter is not empty and + * is not derived from the field name. * @return Selected short name, or "" for none. */ String flag() default ""; diff --git a/sopt/src/main/scala/dagr/sopt/cmdline/ClpArgumentDefinitionPrinting.scala b/sopt/src/main/scala/dagr/sopt/cmdline/ClpArgumentDefinitionPrinting.scala index 60b7a230..45c87974 100644 --- a/sopt/src/main/scala/dagr/sopt/cmdline/ClpArgumentDefinitionPrinting.scala +++ b/sopt/src/main/scala/dagr/sopt/cmdline/ClpArgumentDefinitionPrinting.scala @@ -98,14 +98,26 @@ object ClpArgumentDefinitionPrinting { private val ArgumentColumnWidth: Int = 30 private val DescriptionColumnWidth: Int = 90 + /** Formats a short or long name, depending on the length of the name. */ + private def formatName(name: String, theType: String): String = { + if (name.length == 1) { + val nameType = if (theType == "Boolean") "[true|false]" else theType + "-" + name + " " + nameType + } + else { + val nameType = if (theType == "Boolean") "[=true|false]" else "=" + theType + "--" + name + nameType + } + } + /** Prints the usage for a given argument given its various elements */ private def printArgumentUsage(stringBuilder: StringBuilder, name: String, shortName: String, theType: String, argumentDescription: String): Unit = { // Desired output: "-f Foo, --foo=Foo" and for Booleans, "-f [true|false] --foo=[true|false]" - val (shortType, longType) = if (theType == "Boolean") ("[true|false]","[=true|false]") else (theType, "=" + theType) - val label = new StringBuilder() - if (shortName.nonEmpty) label.append("-" + shortName + " " + shortType + ", ") - label.append("--" + name + longType) - stringBuilder.append(KGRN(label.toString())) + val shortLabel = formatName(shortName, theType) + val longLabel = formatName(name, theType) + if (shortName.length > name.length) throw new IllegalArgumentException(s"Short name '$shortName' is longer than name '$name'") + val label = shortLabel + ", " + longLabel + stringBuilder.append(KGRN(label)) // If the label is short enough, just pad out the column, otherwise wrap to the next line for the description val numSpaces: Int = if (label.length > ArgumentColumnWidth) { diff --git a/sopt/src/main/scala/dagr/sopt/cmdline/ClpReflectiveBuilder.scala b/sopt/src/main/scala/dagr/sopt/cmdline/ClpReflectiveBuilder.scala index 00b4f5a3..e333b100 100644 --- a/sopt/src/main/scala/dagr/sopt/cmdline/ClpReflectiveBuilder.scala +++ b/sopt/src/main/scala/dagr/sopt/cmdline/ClpReflectiveBuilder.scala @@ -147,8 +147,20 @@ private[sopt] class ClpArgument(declaringClass: Class[_], lazy val isSpecial: Boolean = annotation.exists(_.special()) lazy val isSensitive: Boolean = annotation.exists(_.sensitive()) - lazy val longName: String = if (annotation.isDefined && annotation.get.name.nonEmpty) annotation.get.name else StringUtil.camelToGnu(name) - lazy val shortName: String = annotation.map(_.flag()).getOrElse("") + lazy val (shortName, longName) = { + val flagName = annotation.map(_.flag()).getOrElse("") + if (annotation.isDefined && annotation.get.name.nonEmpty) { + if (flagName.length > annotation.get.name.length) { + throw new IllegalStateException("The @arg flag (short name) should be shorter than the @arg name (long name)") + } + (flagName, annotation.get.name) + } else { + // It is ok that the length of the long name when derived from the field name is shorter than shortName since + // we will just swap the two. + val derivedName = StringUtil.camelToGnu(name) + if (flagName.length < derivedName.length) (flagName, derivedName) else (derivedName, flagName) + } + } lazy val doc: String = annotation.map(_.doc()).getOrElse("") lazy val isCommon: Boolean = annotation.exists(_.common()) lazy val minElements: Int = if (isCollection) { diff --git a/sopt/src/test/scala/dagr/sopt/cmdline/CommandLineProgramParserTest.scala b/sopt/src/test/scala/dagr/sopt/cmdline/CommandLineProgramParserTest.scala index 4c3c688b..c1df7cc6 100644 --- a/sopt/src/test/scala/dagr/sopt/cmdline/CommandLineProgramParserTest.scala +++ b/sopt/src/test/scala/dagr/sopt/cmdline/CommandLineProgramParserTest.scala @@ -34,7 +34,6 @@ import org.scalatest.{BeforeAndAfterAll, Inside, OptionValues} import scala.collection.Map import scala.collection.immutable.HashMap -import scala.util.Success //////////////////////////////////////////////////////////////////////////////// // Holds the classes we need for testing. These cannot be inner classes. @@ -312,6 +311,66 @@ private case class BadEnumClass ) private case class SomeDescription (@arg var a: Int = 2) +@clp(description = "", group = classOf[TestGroup], hidden = true) +private case class MultiCharacterShortName +( + @arg(flag="atm") var automatedTellerMachine: String = "Bank" +) + +@clp(description = "", group = classOf[TestGroup], hidden = true) +private case class UseNameInAnnotation +( + @arg(name="atm") var automatedTellerMachine: String = "Bank" +) + +@clp(description = "", group = classOf[TestGroup], hidden = true) +private case class UseVariableName +( + @arg var automatedTellerMachine: String = "Bank" +) + +@clp(description = "", group = classOf[TestGroup], hidden = true) +private case class OneCharNameInAnnotation +( + @arg(name="a") var automatedTellerMachine: String = "Bank" +) + +@clp(description = "", group = classOf[TestGroup], hidden = true) +private case class OneCharVariableName +( + @arg var a: String = "Bank" +) + +@clp(description = "", group = classOf[TestGroup], hidden = true) +private case class FlagIsLongAndAnnotationNameIsShort +( + @arg(flag="long", name="a") var a: String = "Bank" +) + +@clp(description = "", group = classOf[TestGroup], hidden = true) +private case class FlagIsLongAndVariableNameIsShort +( + @arg(flag="long") var a: String = "Bank" +) + +@clp(description = "", group = classOf[TestGroup], hidden = true) +private case class FlagAndAnnotationNameAreA +( + @arg(flag="a", name="a") var a: String = "Bank" +) + +@clp(description = "", group = classOf[TestGroup], hidden = true) +private case class FlagAndVariableNameAreA +( + @arg(flag="a") var a: String = "Bank" +) + +@clp(description = "", group = classOf[TestGroup], hidden = true) +private case class AnnotationNameIsCamelCase +( + @arg(name="camelCase") var a: String = "Bank" +) + //////////////////////////////////////////////////////////////////////////////// // End of Testing CLP classes //////////////////////////////////////////////////////////////////////////////// @@ -576,8 +635,8 @@ with CommandLineParserStrings with CaptureSystemStreams with BeforeAndAfterAll { "CommandLineProgramParser.usage" should "print out no arguments when no arguments are present" in { val usage = parser(classOf[NoArguments]).usage(printCommon = false, withVersion = true, withSpecial = false) - usage should not include (RequiredArguments) - usage should not include (OptionalArguments) + usage should not include RequiredArguments + usage should not include OptionalArguments usage should include (UsagePrefix) } @@ -672,7 +731,52 @@ with CommandLineParserStrings with CaptureSystemStreams with BeforeAndAfterAll { val end = usage.indexOf("") start should be > 0 end should be > 0 - usage.substring(start, end).split("\n").size shouldBe 4 + usage.substring(start, end).split("\n").length shouldBe 4 + } + + it should "format a multi-character flag" in { + val usage = parser(classOf[MultiCharacterShortName]).usage(printCommon = false, withVersion = true, withSpecial = false) + usage should include ("--atm=String, --automated-teller-machine=String") + } + + it should "format using the 'name' in the annotation for the long name when specified" in { + val usage = parser(classOf[UseNameInAnnotation]).usage(printCommon = false, withVersion = true, withSpecial = false) + usage should include ("--atm=String") + usage should not include "--automated-teller-machine" + } + + it should "format using the variable name for the long name when 'name' is not specified" in { + val usage = parser(classOf[UseVariableName]).usage(printCommon = false, withVersion = true, withSpecial = false) + usage should include ("--automated-teller-machine=String") + } + + it should "format using the single-character 'name' in the annotation for the long name when 'name' is specified" in { + val usage = parser(classOf[OneCharNameInAnnotation]).usage(printCommon = false, withVersion = true, withSpecial = false) + usage should include ("-a String") + } + + it should "format using the single-character variable name for the long name when 'name' is not specified" in { + val usage = parser(classOf[OneCharVariableName]).usage(printCommon = false, withVersion = true, withSpecial = false) + usage should include ("-a String") + } + + it should "throw an exception when the length of 'flag' is greater than the length of 'name'" in { + an[Exception] should be thrownBy parser(classOf[FlagIsLongAndAnnotationNameIsShort]).usage(printCommon = false, withVersion = true, withSpecial = false) + } + + it should "format short arguments before long arguments, when the length of 'flag' is longer than the 'name' derived from the field's name" in { + val usage = parser(classOf[FlagIsLongAndVariableNameIsShort]).usage(printCommon = false, withVersion = true, withSpecial = false) + usage should include ("-a String, --long=String") + } + + it should "throw an exception when the values for 'flag' and 'name' are the same" in { + an[Exception] should be thrownBy parser(classOf[FlagAndAnnotationNameAreA]).usage(printCommon = false, withVersion = true, withSpecial = false) + an[Exception] should be thrownBy parser(classOf[FlagAndVariableNameAreA]).usage(printCommon = false, withVersion = true, withSpecial = false) + } + + it should "allow camel casing in the 'name' field" in { + val usage = parser(classOf[AnnotationNameIsCamelCase]).usage(printCommon = false, withVersion = true, withSpecial = false) + usage should include ("--camelCase=String") } "CommandLineProgramParser.parseAndBuild" should "parse multiple positional arguments" in { @@ -1015,8 +1119,15 @@ with CommandLineParserStrings with CaptureSystemStreams with BeforeAndAfterAll { } } + it should "parse a flag (short name) that is more than one character long" in { + val args = Array[String]("--atm", "Vault") + val p = parser(classOf[MultiCharacterShortName]) + inside (p.parseAndBuild(args=args)) { case ParseSuccess() => } + val task = p.instance.get + task.automatedTellerMachine shouldBe "Vault" + } + "CommandLineProgramParser.getCommandLine" should "should return the command line string" in { - val task = new GeneralTestingProgram val args = Array[String]( "--string-set", "Foo", "Bar", "--int-set", "1", "2",