Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow CliktCommand.test to take a vararg #451

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

sschuberth
Copy link
Contributor

For convenience and to better visualize separate options, allow to write

test("--foo", "--bar")

in addition to

test("--foo --bar")

This requires to remove the test(argv: Array<String>, ...) overload as that would have the same JVM function signature.

@sschuberth
Copy link
Contributor Author

This is just a proposal @ajalt, not sure whether you like it as it seems to be a breaking change for explicit users of test(arrayOf("-foo", "--bar"), ...).

@ajalt
Copy link
Owner

ajalt commented Sep 7, 2023

Thanks for the PR.

We can add the vararg version as a separate overload to keep backwards compatibility:

@JvmName("varargTest")
fun CliktCommand.test(
    vararg argv: String,
    stdin: String = "",
    envvars: Map<String, String> = emptyMap(),
    includeSystemEnvvars: Boolean = false,
    ansiLevel: AnsiLevel = AnsiLevel.NONE,
    width: Int = 79,
    height: Int = 24,
): CliktCommandTestResult {
    return test(argv.asList(), stdin, envvars, includeSystemEnvvars, ansiLevel, width, height)
}

A vararg version has the potential to be confusing when called with a single argument (test("-x") calls the single String version which tokenizes the input, but test("-x", "-y") calls the vararg version which doesn't), but I can't think of any situation where the single String version would break fail on any input that would be valid to the varargs, so it should be fine.

For convenience and to better visualize separate options, allow to write

    test("--foo", "--bar")

in addition to

    test("--foo --bar")

To disambiguate from the `test(argv: Array<String>, ...)` overload, set
a custom `JvmName`.
@sschuberth
Copy link
Contributor Author

We can add the vararg version as a separate overload to keep backwards compatibility:

Done.

@ajalt ajalt merged commit e3d6211 into ajalt:master Sep 7, 2023
3 checks passed
@ajalt
Copy link
Owner

ajalt commented Sep 7, 2023

Thanks!

@sschuberth sschuberth deleted the test-vararg branch September 8, 2023 05:35
sschuberth added a commit to oss-review-toolkit/ort that referenced this pull request Sep 13, 2023
….test`

Note that a future version of clikt will also support a vararg for `test`
for convenience [1]. According code can be migrated once that version is
available.

[1]: ajalt/clikt#451

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
sschuberth added a commit to oss-review-toolkit/ort that referenced this pull request Sep 13, 2023
….test`

Note that a future version of clikt will also support a vararg for `test`
for convenience [1]. According code can be migrated once that version is
available.

[1]: ajalt/clikt#451

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants