Skip to content

Commit

Permalink
Fix error message when lazily referencing required options (#431)
Browse files Browse the repository at this point in the history
In order to support referencing other options in `defaultLazy`, we call `finalize` twice if the first call caused an IllegalStateException. The second time happens after all other options are finalized, so as long as there are no reference cycles or chains, the second call will succeed.

In order to support MultiUsageError, we have to collect all UsageErrors until after the second finalization round.

The regression happened because we were throwing IllegalStateExceptions that occur in the second round even if there were deferred UsageErrors.
  • Loading branch information
ajalt committed Jul 24, 2023
1 parent 5067136 commit 9ec4a88
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 3 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## Unreleased
### Fixed
- Fixed incorrect error message when a `defaultLazy` option referenced a `required` option. ([#430](https://github.com/ajalt/clikt/issues/430))

## 4.1.0
### Added
- Added `MordantHelpFormatter.renderAttachedOptionValue` that you can override to change how option values are shown, e.g. if you want option to show as `--option <value>` instead of `--option=<value>`. ([#416](https://github.com/ajalt/clikt/issues/416))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,15 @@ internal fun finalizeOptions(
for (o in retries) {
try {
o.finalize(context, emptyList())
} catch (e: IllegalStateException) {
// If we had an earlier usage error, throw that instead of the ISE
MultiUsageError.buildOrNull(errors)?.let { throw it }
throw e
} catch (e: UsageError) {
errors += e
context.errorEncountered = true
}
}

MultiUsageError.buildOrNull(errors)?.let {
throw it
}
MultiUsageError.buildOrNull(errors)?.let { throw it }
}
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,20 @@ class OptionTest {
C().parse("")
}


@Test
@JsName("defaultLazy_option_referencing_required_option")
fun `defaultLazy option referencing required option`() {
class C : TestCommand(called = false) {
val y by option().defaultLazy { x }
val x by option().required()
}

shouldThrow<MissingOption> {
C().parse("")
}.formattedMessage shouldBe "missing option --x"
}

@Test
@JsName("defaultLazy_option_referencing_argument")
fun `defaultLazy option referencing argument`() {
Expand Down

0 comments on commit 9ec4a88

Please sign in to comment.