Only finalize parameters once with multiple subcommands

CHANGELOG.md

@@ -7,6 +7,9 @@
 ### Changed
 - When `printHelpOnEmptyArgs` is `true` and no arguments are present, or when `invokeWithoutSubcommand` is `false` and no subcommand is present, `CliktCommand.main` will now exit with status code 1 rather than 0. 
 
+### Fixed
+- Fixed option values being reset when calling multiple subcommands with `allowMultipleSubcommands=true` ([#190](https://github.com/ajalt/clikt/issues/190))
+
 ## [2.7.1] - 2020-05-19
 ### Fixed
 - Fixed NPE thrown when in some cases when using `defaultByName` ([#179](https://github.com/ajalt/clikt/issues/179))

clikt/src/commonMain/kotlin/com/github/ajalt/clikt/parsers/Parser.kt

@@ -111,61 +111,64 @@ internal object Parser {
         val invocationsByOptionByGroup = invocationsByGroup.mapValues { (_, invs) -> invs.groupBy({ it.opt }, { it.inv }).filterKeys { it !is EagerOption } }
 
         try {
-            // Finalize eager options
-            invocationsByOption.forEach { (o, inv) -> if (o is EagerOption) o.finalize(context, inv) }
-
-            // Finalize un-grouped options that occurred on the command line
-            invocationsByOptionByGroup[null]?.forEach { (o, inv) -> o.finalize(context, inv) }
-
-            // Finalize un-grouped options not provided on the command line so that they can apply default values etc.
-            command._options.forEach { o ->
-                if (o !is EagerOption && o !in invocationsByOption && (o as? GroupableOption)?.parameterGroup == null) {
-                    o.finalize(context, emptyList())
+            // Finalize and validate everything as long as we aren't resuming a parse for multiple subcommands
+            if (canRun) {
+                // Finalize eager options
+                invocationsByOption.forEach { (o, inv) -> if (o is EagerOption) o.finalize(context, inv) }
+
+                // Finalize un-grouped options that occurred on the command line
+                invocationsByOptionByGroup[null]?.forEach { (o, inv) -> o.finalize(context, inv) }
+
+                // Finalize un-grouped options not provided on the command line so that they can apply default values etc.
+                command._options.forEach { o ->
+                    if (o !is EagerOption && o !in invocationsByOption && (o as? GroupableOption)?.parameterGroup == null) {
+                        o.finalize(context, emptyList())
+                    }
                 }
-            }
 
-            // Finalize option groups after other options so that the groups can use their values
-            invocationsByOptionByGroup.forEach { (group, invocations) ->
-                group?.finalize(context, invocations)
-            }
+                // Finalize option groups after other options so that the groups can use their values
+                invocationsByOptionByGroup.forEach { (group, invocations) ->
+                    group?.finalize(context, invocations)
+                }
 
-            // Finalize groups with no invocations
-            command._groups.forEach { if (it !in invocationsByGroup) it.finalize(context, emptyMap()) }
-
-            val (excess, parsedArgs) = parseArguments(positionalArgs, arguments)
-            parsedArgs.forEach { (it, v) -> it.finalize(context, v) }
-
-            if (excess > 0) {
-                if (hasMultipleSubAncestor) {
-                    i = tokens.size - excess
-                } else if (excess == 1 && subcommands.isNotEmpty()) {
-                    val actual = positionalArgs.last()
-                    throw NoSuchSubcommand(actual, context.correctionSuggestor(actual, subcommands.keys.toList()))
-                } else {
-                    val actual = positionalArgs.takeLast(excess).joinToString(" ", limit = 3, prefix = "(", postfix = ")")
-                    throw UsageError("Got unexpected extra argument${if (excess == 1) "" else "s"} $actual")
+                // Finalize groups with no invocations
+                command._groups.forEach { if (it !in invocationsByGroup) it.finalize(context, emptyMap()) }
+
+                val (excess, parsedArgs) = parseArguments(positionalArgs, arguments)
+                parsedArgs.forEach { (it, v) -> it.finalize(context, v) }
+                if (excess > 0) {
+                    if (hasMultipleSubAncestor) {
+                        i = tokens.size - excess
+                    } else if (excess == 1 && subcommands.isNotEmpty()) {
+                        val actual = positionalArgs.last()
+                        throw NoSuchSubcommand(actual, context.correctionSuggestor(actual, subcommands.keys.toList()))
+                    } else {
+                        val actual = positionalArgs.takeLast(excess).joinToString(" ", limit = 3, prefix = "(", postfix = ")")
+                        throw UsageError("Got unexpected extra argument${if (excess == 1) "" else "s"} $actual")
+                    }
                 }
-            }
 
-            // Now that all parameters have been finalized, we can validate everything
-            command._options.forEach { o -> if ((o as? GroupableOption)?.parameterGroup == null) o.postValidate(context) }
-            command._groups.forEach { it.postValidate(context) }
-            command._arguments.forEach { it.postValidate(context) }
 
-            if (subcommand == null && subcommands.isNotEmpty() && !command.invokeWithoutSubcommand) {
-                throw PrintHelpMessage(command, error = true)
-            }
+                // Now that all parameters have been finalized, we can validate everything
+                command._options.forEach { o -> if ((o as? GroupableOption)?.parameterGroup == null) o.postValidate(context) }
+                command._groups.forEach { it.postValidate(context) }
+                command._arguments.forEach { it.postValidate(context) }
 
-            command.currentContext.invokedSubcommand = subcommand
-            if (command.currentContext.printExtraMessages) {
-                val console = command.currentContext.console
-                for (warning in command.messages) {
-                    console.print(warning, error = true)
-                    console.print(console.lineSeparator, error = true)
+                if (subcommand == null && subcommands.isNotEmpty() && !command.invokeWithoutSubcommand) {
+                    throw PrintHelpMessage(command, error = true)
                 }
-            }
 
-            if (canRun) command.run()
+                command.currentContext.invokedSubcommand = subcommand
+                if (command.currentContext.printExtraMessages) {
+                    val console = command.currentContext.console
+                    for (warning in command.messages) {
+                        console.print(warning, error = true)
+                        console.print(console.lineSeparator, error = true)
+                    }
+                }
+
+                command.run()
+            }
         } catch (e: UsageError) {
             // Augment usage errors with the current context if they don't have one
             if (e.context == null) e.context = context

clikt/src/commonTest/kotlin/com/github/ajalt/clikt/core/CliktCommandTest.kt

@@ -231,94 +231,6 @@ class CliktCommandTest {
         c.registeredParameterGroups() shouldBe listOf(g)
     }
 
-    @Test
-    @JsName("subcommand_cycle")
-    fun `subcommand cycle`() {
-        val root = TestCommand(called = false)
-        val a = TestCommand(called = false, name = "a")
-        val b = TestCommand(called = false)
-
-        shouldThrow<IllegalStateException> {
-            root.subcommands(a.subcommands(b.subcommands(a))).parse("a b a")
-        }.message shouldBe "Command a already registered"
-    }
-
-    @Test
-    @JsName("multiple_subcommands")
-    fun `multiple subcommands`() = forall(
-            row("foo a", 1, 0, "a", null, null),
-            row("foo a foo b", 2, 0, "b", null, null),
-            row("bar a", 0, 1, null, null, "a"),
-            row("bar a bar b", 0, 2, null, null, "b"),
-            row("bar --opt=o a", 0, 1, null, "o", "a"),
-            row("foo a bar --opt=o b foo c bar d", 2, 2, "c", null, "d"),
-            row("foo a bar b foo c bar --opt=o d", 2, 2, "c", "o", "d")
-    ) { argv, fc, bc, fa, bo, ba ->
-        val foo = MultiSub1(count = fc)
-        val bar = MultiSub2(count = bc)
-        val c = TestCommand(allowMultipleSubcommands = true).subcommands(foo, bar, TestCommand(called = false))
-        c.parse(argv)
-        if (fc > 0) foo.arg shouldBe fa
-        bar.opt shouldBe bo
-        if (bc > 0) bar.arg shouldBe ba
-    }
-
-    @Test
-    @JsName("multiple_subcommands_with_nesting")
-    fun `multiple subcommands with nesting`() {
-        val foo = MultiSub1(count = 2)
-        val bar = MultiSub2(count = 2)
-        val c = TestCommand(allowMultipleSubcommands = true).subcommands(foo.subcommands(bar))
-        c.parse("foo f1 bar --opt=1 b1 foo f2 bar b2")
-        foo.arg shouldBe "f2"
-        bar.opt shouldBe null
-        bar.arg shouldBe "b2"
-    }
-
-    @Test
-    @JsName("multiple_subcommands_nesting_the_same_name")
-    fun `multiple subcommands nesting the same name`() {
-        val bar1 = MultiSub2(count = 2)
-        val bar2 = MultiSub2(count = 2)
-        val c = TestCommand(allowMultipleSubcommands = true).subcommands(bar1.subcommands(bar2))
-        c.parse("bar a11 bar a12 bar a12 bar --opt=o a22")
-        bar1.arg shouldBe "a12"
-        bar2.opt shouldBe "o"
-        bar2.arg shouldBe "a22"
-    }
-
-    @Test
-    @JsName("multiple_subcommands_with_varargs")
-    fun `multiple subcommands with varargs`() = forall(
-            row("foo f1 baz", 1, 1, "f1", emptyList()),
-            row("foo f1 foo f2 baz", 2, 1, "f2", emptyList()),
-            row("baz foo", 0, 1, "", listOf("foo")),
-            row("baz foo baz foo", 0, 1, "", listOf("foo", "baz", "foo")),
-            row("foo f1 baz foo f2", 1, 1, "f1", listOf("foo", "f2"))
-    ) { argv, fc, bc, fa, ba ->
-        class Baz : TestCommand(name = "baz", count = bc) {
-            val arg by argument().multiple()
-        }
-
-        val foo = MultiSub1(count = fc)
-        val baz = Baz()
-        val c = TestCommand(allowMultipleSubcommands = true).subcommands(foo, baz)
-        c.parse(argv)
-
-        if (fc > 0) foo.arg shouldBe fa
-        baz.arg shouldBe ba
-    }
-
-    @Test
-    @JsName("multiple_subcommands_nesting_multiple_subcommands")
-    fun `multiple subcommands nesting multiple subcommands`() {
-        val c = TestCommand(allowMultipleSubcommands = true)
-                .subcommands(TestCommand(allowMultipleSubcommands = true))
-        shouldThrow<IllegalArgumentException> {
-            c.parse("")
-        }.message shouldContain "allowMultipleSubcommands"
-    }
-
     @Test
     @JsName("treat_unknown_options_as_arguments")
     fun `treat unknown options as arguments`() {
@@ -352,12 +264,3 @@ class CliktCommandTest {
         }.message shouldBe "no such option: \"-g\"."
     }
 }
-
-private class MultiSub1(count: Int) : TestCommand(name = "foo", count = count) {
-    val arg by argument()
-}
-
-private class MultiSub2(count: Int) : TestCommand(name = "bar", count = count) {
-    val opt by option()
-    val arg by argument()
-}

clikt/src/commonTest/kotlin/com/github/ajalt/clikt/parameters/SubcommandTest.kt

@@ -8,10 +8,12 @@ import com.github.ajalt.clikt.parameters.arguments.argument
 import com.github.ajalt.clikt.parameters.arguments.multiple
 import com.github.ajalt.clikt.parameters.arguments.pair
 import com.github.ajalt.clikt.parameters.options.option
+import com.github.ajalt.clikt.parameters.options.required
 import com.github.ajalt.clikt.testing.TestCommand
 import io.kotest.assertions.throwables.shouldThrow
 import io.kotest.data.forall
 import io.kotest.matchers.shouldBe
+import io.kotest.matchers.string.shouldContain
 import io.kotest.tables.row
 import kotlin.js.JsName
 import kotlin.test.Test
@@ -270,4 +272,131 @@ class SubcommandTest {
                     .parse(argv)
         }.message shouldBe message
     }
+
+
+    @Test
+    @JsName("subcommand_cycle")
+    fun `subcommand cycle`() {
+        val root = TestCommand(called = false)
+        val a = TestCommand(called = false, name = "a")
+        val b = TestCommand(called = false)
+
+        shouldThrow<IllegalStateException> {
+            root.subcommands(a.subcommands(b.subcommands(a))).parse("a b a")
+        }.message shouldBe "Command a already registered"
+    }
+
+    @Test
+    fun allowMultipleSubcommands() = forall(
+            row("foo a", 1, 0, "a", null, null),
+            row("foo a foo b", 2, 0, "b", null, null),
+            row("bar a", 0, 1, null, null, "a"),
+            row("bar a bar b", 0, 2, null, null, "b"),
+            row("bar --opt=o a", 0, 1, null, "o", "a"),
+            row("foo a bar --opt=o b foo c bar d", 2, 2, "c", null, "d"),
+            row("foo a bar b foo c bar --opt=o d", 2, 2, "c", "o", "d")
+    ) { argv, fc, bc, fa, bo, ba ->
+        val foo = MultiSub1(count = fc)
+        val bar = MultiSub2(count = bc)
+        val c = TestCommand(allowMultipleSubcommands = true).subcommands(foo, bar, TestCommand(called = false))
+        c.parse(argv)
+        if (fc > 0) foo.arg shouldBe fa
+        bar.opt shouldBe bo
+        if (bc > 0) bar.arg shouldBe ba
+    }
+
+    @Test
+    @JsName("multiple_subcommands_with_nesting")
+    fun `multiple subcommands with nesting`() {
+        val foo = MultiSub1(count = 2)
+        val bar = MultiSub2(count = 2)
+        val c = TestCommand(allowMultipleSubcommands = true).subcommands(foo.subcommands(bar))
+        c.parse("foo f1 bar --opt=1 b1 foo f2 bar b2")
+        foo.arg shouldBe "f2"
+        bar.opt shouldBe null
+        bar.arg shouldBe "b2"
+    }
+
+    @Test
+    @JsName("multiple_subcommands_nesting_the_same_name")
+    fun `multiple subcommands nesting the same name`() {
+        val bar1 = MultiSub2(count = 2)
+        val bar2 = MultiSub2(count = 2)
+        val c = TestCommand(allowMultipleSubcommands = true).subcommands(bar1.subcommands(bar2))
+        c.parse("bar a11 bar a12 bar a12 bar --opt=o a22")
+        bar1.arg shouldBe "a12"
+        bar2.opt shouldBe "o"
+        bar2.arg shouldBe "a22"
+    }
+
+    @Test
+    @JsName("multiple_subcommands_with_varargs")
+    fun `multiple subcommands with varargs`() = forall(
+            row("foo f1 baz", 1, 1, "f1", emptyList()),
+            row("foo f1 foo f2 baz", 2, 1, "f2", emptyList()),
+            row("baz foo", 0, 1, "", listOf("foo")),
+            row("baz foo baz foo", 0, 1, "", listOf("foo", "baz", "foo")),
+            row("foo f1 baz foo f2", 1, 1, "f1", listOf("foo", "f2"))
+    ) { argv, fc, bc, fa, ba ->
+        class Baz : TestCommand(name = "baz", count = bc) {
+            val arg by argument().multiple()
+        }
+
+        val foo = MultiSub1(count = fc)
+        val baz = Baz()
+        val c = TestCommand(allowMultipleSubcommands = true).subcommands(foo, baz)
+        c.parse(argv)
+
+        if (fc > 0) foo.arg shouldBe fa
+        baz.arg shouldBe ba
+    }
+
+    @Test
+    @JsName("multiple_subcommands_nesting_multiple_subcommands")
+    fun `multiple subcommands nesting multiple subcommands`() {
+        val c = TestCommand(allowMultipleSubcommands = true)
+                .subcommands(TestCommand(allowMultipleSubcommands = true))
+        shouldThrow<IllegalArgumentException> {
+            c.parse("")
+        }.message shouldContain "allowMultipleSubcommands"
+    }
+
+    @Test
+    @JsName("multiple_subcommands_root_option")
+    fun `multiple subcommands with root option`() {
+        class C : TestCommand(count = 1, allowMultipleSubcommands = true) {
+            val x by option()
+
+            override fun run_() {
+                x shouldBe "xx"
+            }
+        }
+
+        val c = C()
+        c.subcommands(MultiSub1(1), MultiSub2(1)).parse("--x=xx foo 1 bar 2")
+        c.x shouldBe "xx"
+    }
+
+    @Test
+    @JsName("multiple_subcommands_required_option")
+    fun `multiple subcommands with required option`() {
+        class C : TestCommand(count = 1, allowMultipleSubcommands = true) {
+            val x by option().required()
+
+            override fun run_() {
+                x shouldBe "xx"
+            }
+        }
+
+        C().subcommands(MultiSub1(1), MultiSub2(1)).parse("--x=xx foo 1 bar 2")
+    }
+}
+
+private class MultiSub1(count: Int) : TestCommand(name = "foo", count = count) {
+    val arg by argument()
+}
+
+private class MultiSub2(count: Int) : TestCommand(name = "bar", count = count) {
+    val opt by option()
+    val arg by argument()
 }