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

feat(completion): Added Command support for CompletionCandidate #79

Closed
wants to merge 1 commit into from

Conversation

pdelagrave
Copy link

  • New CompletionCandidate.Command allowing to generate candidates from an arbitrary command/shell expression.
  • Updated CliktCommand.argument() to support an optional CompletionCandidate argument.

Necessary to call any command to generate values for autocomplete. My use case was this CLI using clikt having a cli do VALUE command which values can be autocompleted by calling cli list.

Let me know what you think.

Sidenote: may I suggest this convention/tool to help manage the changelog from the commit messages:
https://github.com/clog-tool/clog-cli

Awesome project by the way, thanks!

- New `CompletionCandidate.Command` allowing to generate candidates from an arbitrary command/shell expression.
- Updated `CliktCommand.argument()` to support an optional `CompletionCandidate` argument.
@ajalt
Copy link
Owner

ajalt commented Jul 11, 2019

I don't fully understand the use case. Clikt already has fixed string completion, how is this different?

@pdelagrave
Copy link
Author

Rather than providing a fixed set of words at compile time (CompletionCandidate.Fixed), a "command" can be provided so the words available to the user for completion are dynamic at use time. It is executed when the use hits [TAB][TAB].

Copy link
Owner

@ajalt ajalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main reservation for this functionality is that it will affect portability across shells. Right now a Clikt program generates completion that is usable in both zsh and bash, but allowing user supplied shell code could cause the generated script to only work on one of them. This would be an even larger problem if we add support for more shells in the future.

@@ -196,6 +196,9 @@ internal object CompletionGenerator {
completion.candidates.joinTo(this, " ")
append("' -- \"\${word}\"))\n")
}
is CompletionCandidates.Command -> {
append(" COMPREPLY=(\$(compgen -W \"`${completion.command}`\" -- \"\${word}\"))\n")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could cause quoting issues. I think a better solution might be to put the command in a separate function and use compgen -F.

@pdelagrave
Copy link
Author

Offering the flexibility of inserting an arbitrary shell expression into the generated completion script does come with the risk for the library user to break the portability and introduce shell syntax errors.
The cliché holds true, with great power comes great responsibility.

Generating cross compatible shell completion scripts is quite a feat, doing so successfully requires at least to tightly control the library user's input to the generator. This come at the cost of not allowing for much flexibility which strongly lower the value we get out of it.

My current use case is to reuse an existing CliktCommand from the same app to generate the completion values for another CliktCommand, so it would be possible to generate 100% valid shell code for that without letting the user directly input a shell expression. Although this would require a lot more effort than this simple PR, and would still limit the user to calling only internal existing commands.

Many of the completion scripts I wrote in the past are using random unrelated commands and I assume it could be the same for other Clikt users, hence the proposition to let the user free to input whatever command is needed.

Would wrapping the provided shell expression in a function like you suggested be safe enough? Let me know if you have other reservations.

Thanks

Copy link
Owner

@ajalt ajalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapping the shell code in a function would prevent quoting issues, but wouldn't do anything for shell compatibility.

*/
@Suppress("unused")
fun CliktCommand.argument(
name: String = "",
help: String = "",
helpTags: Map<String, String> = emptyMap()
helpTags: Map<String, String> = emptyMap(),
completionCandidates: CompletionCandidates = CompletionCandidates.None
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work in some cases; functions like file will overwrite the value set by the user.

@pdelagrave
Copy link
Author

A more sustainable approach to keep the completion script compatible with many shells while leaving lots of flexibility to the user as how and what gets generated: the shell script could be minimalistic and offload the work back to the cli. For example in gsutil: https://github.com/GoogleCloudPlatform/gsutil

@ajalt
Copy link
Owner

ajalt commented Jul 12, 2019

Calling back into the program would be more powerful, but I decided against it due to JVM startup times. Running a JAR with an empty main function takes more than .5 seconds on my machine, which I think is too slow to run every time you press tab.

@pdelagrave
Copy link
Author

What about data class Command(val command: List<String>) : CompletionCandidates()?
The user would have to specify the command and its arguments in an List allowing clikt to ensure shell compatibility by controlling the invocation. The user will be responsible for specifying arguments such as calling the program will output a space separated list of string.

@ajalt
Copy link
Owner

ajalt commented Jan 22, 2020

Closing in favor of #102

@ajalt ajalt closed this Jan 22, 2020
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.

None yet

2 participants