From 81db51b5445d0c3c962dfbc19fa91d08aabe1abe 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". Make it ok for the 'flag' and derived 'name' are the same, but only print out one: `@arg(flag="a") val a: T` will be "-a T". Updated the documentation for ArgAnnotation for all the above. --- .../java/dagr/sopt/cmdline/ArgAnnotation.java | 13 +- .../ClpArgumentDefinitionPrinting.scala | 24 +++- .../sopt/cmdline/ClpReflectiveBuilder.scala | 17 ++- .../CommandLineProgramParserTest.scala | 125 +++++++++++++++++- 4 files changed, 163 insertions(+), 16 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..4881e54c 100644 --- a/sopt/src/main/java/dagr/sopt/cmdline/ArgAnnotation.java +++ b/sopt/src/main/java/dagr/sopt/cmdline/ArgAnnotation.java @@ -45,15 +45,20 @@ * prefixed on the command-line with a double dash (--). If not specified * then default is used, which is to translate the field name into a GNU style * option name by breaking words on camel case, joining words back together with - * hyphens, and converting to lower case (e.g. myThing => my-thing). + * hyphens, and converting to lower case (e.g. myThing => my-thing). The + * resulting value should be different than {@link #flag()}. * @return Selected full name, or "" to use the default. */ 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 have length less than or equal to {@link #name()} if the latter + * is not empty and is not derived from the field name. Regardless, they should + * not have the same value. * @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..376da5a7 100644 --- a/sopt/src/main/scala/dagr/sopt/cmdline/ClpArgumentDefinitionPrinting.scala +++ b/sopt/src/main/scala/dagr/sopt/cmdline/ClpArgumentDefinitionPrinting.scala @@ -98,14 +98,28 @@ 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 label = if (name == "") shortLabel else { + val longLabel = formatName(name, theType) + if (shortName.length > name.length) throw new IllegalArgumentException(s"Short name '$shortName' is longer than name '$name'") + 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..785e3852 100644 --- a/sopt/src/main/scala/dagr/sopt/cmdline/ClpReflectiveBuilder.scala +++ b/sopt/src/main/scala/dagr/sopt/cmdline/ClpReflectiveBuilder.scala @@ -147,8 +147,21 @@ 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 (derivedName == flagName) (flagName, "") + else 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..e86e4e2d 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,56 @@ 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 'name' is 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) + } + + 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") + } + + it should "allow the flag to be specified as the same as 'name' when 'name' is derived from the field's name " in { + val usage = parser(classOf[FlagAndVariableNameAreA]).usage(printCommon = false, withVersion = true, withSpecial = false) + usage should include ("-a String") } "CommandLineProgramParser.parseAndBuild" should "parse multiple positional arguments" in { @@ -1015,8 +1123,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",