Improve the command for printing completion scripts #1998

bartekpacia · 2024-10-28T01:21:27Z

Which issue(s) this PR fixes

This is a feature PR that resolves issue #1904.

Special notes for your reviewer

I would appreciate it if you check out the code locally, build it, and test the shell completions yourself.

I did that myself, but the more testing, the better. Also, if you have any ideas on how to test this better, I'd love to hear them.

Release Notes

Rename the generate-completion flag to simply completion. This makes our behavior in this matter consistent with the other very popular CLI library for Go – spf13/cobra.
The completion scripts now include your CLI app name and are ready to be used right away.

To enable completion only for the current shell Zsh session:

. <(./my-awesome-app completion zsh

To enable completions permanently by placing the completion scripts into the standard completions directory:

my-awesome-app completion zsh > /opt/homebrew/share/zsh/site-functions/_my-awesome-app

Please note that for completion to work, your top-level cli.Command name and your binary name must be the same.

completion.go

command.go

bartekpacia · 2024-11-02T21:44:14Z

I'm having trouble understanding why it fails.

Running FLAGS="--walk docs/v3/" make gfmrun works without problem for me.

Will appreciate help here @dearchap @meatballhat @abitrolly

dearchap · 2024-11-03T13:14:22Z

examples/simpletask/main.go

@@ -0,0 +1,70 @@
+package main


I dont see the value of this example. Its not really doing anything. If you want to really test this move it into examples_test.go or call it func ExampleCompletion(...) in completion_test.go

Hi, I do see its value, it's a simple yet quite realistic CLI app. It's quite useful for testing shell completions, because it has a few subcommand and sets EnableShellCompletion: true.

Maybe I can modify example-cli or example-hello-world and add a few subcommand and EnableShellCompletion: true there?

I find complete examples very useful. Sometimes you just need a bit of working code to debug something that doesn't work anymore. If completions break when migrating from v2 to v3, then using this v3 code as a reference, I could find the cause much faster.

Complete working examples are also useful for training AI.

It needs some better organization, though. Maybe even numbers to sort contents in the order people usually learn the library. By most frequent use cases.

https://github.com/urfave/cli/tree/ef45965eeb9c1122885fafa4a391b6be6a674f3d/examples

Thats where the https://github.com/urfave/cli/blob/ef45965eeb9c1122885fafa4a391b6be6a674f3d/examples_test.go comes into play.

@dearchap https://github.com/urfave/cli/blob/ef45965eeb9c1122885fafa4a391b6be6a674f3d/examples_test.go is not discoverable, and also they do not work if copy-pasted.

Yeah, the main reason why I put this sample in a new file is because examples_test is not easily runnable.

I agree with @abitrolly comment that it's be nice to have a single place for more "full app" examples.

example/simpletask.go doesn't tell anything to the person who browses examples. For the sake of bikeshedding examples/commands.go may be a better name.

I understand that this example can be used to test command completions, but because it is not used actually used in tests, I would suggest to submit it in a separate PR, where we could also discuss how to integrate it with what we have in docs.

Good idea, I agree. I will add the simpletask example in a separate PR.

Oh, actually I think I have another reason why I like /usr/bin/env bash.

For example on macOS, the /bin/bash is a 20-years old bash3, and almost everyone using bash installs a modern bash5 with Homebrew.

Output on my machine:

$ which -a bash /opt/homebrew/bin/bash /bin/bash

So I think hardcoding /bin/bash in this case will always default to the pre-installed bash3, which I think is not so nice.

wdyt?

completion.go

abitrolly · 2024-11-04T07:12:56Z

I'm having trouble understanding why it fails.

Running FLAGS="--walk docs/v3/" make gfmrun works without problem for me.

I am not familiar with this piece of code. And don't see any failures. I guess it is fixed.

What concerns me is how much to the binary size is added by the completion feature and its templates? I guess it is non-optional, right?

dearchap · 2024-11-04T13:36:43Z

@abitrolly All the template code is already in current binary. So @bartekpacia 's work doesnt really change.

Co-authored-by: dearchap <[email protected]>

dearchap · 2024-11-10T13:50:40Z

@abitrolly what I meant is that removing embedFS and templating it in code doesnt change the size of the overall cli binary. I am ok with either approach unless there is a good reason one way or the other

abitrolly · 2024-11-11T08:58:57Z

@bartekpacia I am not okay with moving code sections without really good reasons, because comparing historical changes in these pieces becomes really hard.

bartekpacia · 2024-11-11T16:50:11Z

Thanks for the review and discussion.

I don't agree with your opinion @abitrolly about mixing Go+Shell but it's not really an important thing so let's not bikeshed. I'll move completions back to their respective files.

I plan to spend some time soon on improving the completions, so if this proves to be annoying we can revisit it down the road.

This reverts commit cffef65.

bartekpacia · 2024-11-23T21:45:42Z

@Juneezee @dearchap @abitrolly i moved completion script strings back into individual files. I think this PR is good to be merged now.

abitrolly

I left some minor comments for the things I could review for the time being. It would be realistic to merge them if submitted in a separate PRs, because the rest of the diff is beyond my ability to review right now.

help.go

abitrolly · 2024-11-24T09:31:03Z

examples/simpletask/main.go

@@ -0,0 +1,70 @@
+package main


example/simpletask.go doesn't tell anything to the person who browses examples. For the sake of bikeshedding examples/commands.go may be a better name.

I understand that this example can be used to test command completions, but because it is not used actually used in tests, I would suggest to submit it in a separate PR, where we could also discuss how to integrate it with what we have in docs.

autocomplete/bash_autocomplete

abitrolly · 2024-11-24T19:06:31Z

Rename the generate-completion flag to simply completion.

I am not aware how this feature works.

Is it enabled by default?
Does it show up in the list of user configured commands by default?
Can users choose the name they are prefer for this completion command?

bartekpacia · 2024-11-24T19:11:04Z

This PR does 2 things:

renames an existing generate-completion to be completion (in-line with Cobra and other popular CLI libraries)
the completion script printed by this command now contains the app name, and is ready to be pasted in the completion directory without any kind of manual editing.

No other behavior changes are expected.

abitrolly · 2024-11-24T20:11:35Z

This PR does 2 things:

Maybe make 2 PRs making 1 thing, so that we can finally merge at least something already. :D

dearchap · 2024-11-24T20:13:36Z

completion.go

-		"zsh":  getCompletion("autocomplete/zsh_autocomplete"),
-		"fish": func(c *Command) (string, error) {
+		"bash": func(c *Command, appName string) (string, error) {
+			b, err := autoCompleteFS.ReadFile("autocomplete/bash_autocomplete")


i wonder if we can ignore the error since we are loading from embedFS. Saves us from writing unit tests as well.

nice catch! I think it's not worth worrying about the error here

I just decided to not additionaly wrap the errors to minimize amount of changes.

I think it's fine now.

dearchap

Please try to add additional tests or remove err in the lookup.

abitrolly

I haven't tested Go code, and can not give it a proper review, but all questions that I had had been addressed, so no reason for me to hold this back. Except, maybe to squash the history, or rebase to a minimal set of commits to avoid stumbling into reverts while blaming the history.

bartekpacia · 2024-11-24T20:37:38Z

thanks for the review everyone :)

I will of course "squash and merge" this PR to not butcher history

dearchap reviewed Oct 28, 2024

View reviewed changes

completion.go Outdated Show resolved Hide resolved

dearchap reviewed Oct 28, 2024

View reviewed changes

command.go Outdated Show resolved Hide resolved

bartekpacia force-pushed the bartekpacia/feature/expose_completion_scripts branch 3 times, most recently from 4c63582 to ed55e57 Compare October 30, 2024 23:13

bartekpacia added 10 commits November 3, 2024 10:31

fix linter warning

fc30c1e

update autocomplete script with useful link

ed434ac

start impl

bf5d792

delete zsh_autocomplete file

717b07c

create a simple app to test out completion

f554abb

help.go printFlagSuggestions(): add small comment

f81b2cb

fix tests

4dcd89b

zsh completion: add comments to help understand code

c84b5ca

move all autocomplete shell files into Go code strings

cffef65

impl

24524da

bartekpacia force-pushed the bartekpacia/feature/expose_completion_scripts branch from ed55e57 to 24524da Compare November 3, 2024 09:31

bartekpacia marked this pull request as ready for review November 3, 2024 09:35

bartekpacia requested a review from a team as a code owner November 3, 2024 09:35

dearchap reviewed Nov 3, 2024

View reviewed changes

completion.go Outdated Show resolved Hide resolved

bartekpacia and others added 2 commits November 5, 2024 21:05

use osArgs instead of cmd.Name

319a061

Co-authored-by: dearchap <[email protected]>

bash completion: fix function names

cc5c66a

bartekpacia requested a review from dearchap November 5, 2024 20:20

bartekpacia assigned abitrolly Nov 5, 2024

bartekpacia requested review from meatballhat and a team November 5, 2024 20:20

remove redundant variable completionFlagName

d211484

Revert "move all autocomplete shell files into Go code strings"

e27feb4

This reverts commit cffef65.

bartekpacia force-pushed the bartekpacia/feature/expose_completion_scripts branch from 2da1d53 to 8a0b42d Compare November 23, 2024 21:37

move scripts back to files

9b41c34

bartekpacia force-pushed the bartekpacia/feature/expose_completion_scripts branch from 8a0b42d to 9b41c34 Compare November 23, 2024 21:42

bartekpacia requested review from abitrolly and Juneezee November 23, 2024 21:45

abitrolly reviewed Nov 24, 2024

View reviewed changes

bartekpacia added 3 commits November 24, 2024 19:03

revert var name change

de30973

delete simpletask example program

afd23df

use TrimLeft instead of TrimPrefix

4f29af7

bartekpacia force-pushed the bartekpacia/feature/expose_completion_scripts branch from b901b8d to 4f29af7 Compare November 24, 2024 18:07

bartekpacia requested a review from abitrolly November 24, 2024 18:07

fix mistake in shebang

19c9c1e

dearchap reviewed Nov 24, 2024

View reviewed changes

dearchap approved these changes Nov 24, 2024

View reviewed changes

dont wrap error

4f2cec4

bartekpacia requested a review from dearchap November 24, 2024 20:21

bring back /bin/bash shebang

c5ff653

abitrolly approved these changes Nov 24, 2024

View reviewed changes

bartekpacia merged commit 588ada5 into main Nov 24, 2024
15 checks passed

bartekpacia deleted the bartekpacia/feature/expose_completion_scripts branch November 24, 2024 21:22

bartekpacia mentioned this pull request Nov 24, 2024

Use cmd.Name when generating completion script #2012

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the command for printing completion scripts #1998

Improve the command for printing completion scripts #1998

bartekpacia commented Oct 28, 2024

bartekpacia commented Nov 2, 2024

dearchap Nov 3, 2024

bartekpacia Nov 5, 2024 •

edited

Loading

abitrolly Nov 10, 2024

dearchap Nov 10, 2024

abitrolly Nov 11, 2024

bartekpacia Nov 11, 2024

abitrolly Nov 24, 2024

bartekpacia Nov 24, 2024

bartekpacia Nov 24, 2024 •

edited

Loading

abitrolly commented Nov 4, 2024

dearchap commented Nov 4, 2024

dearchap commented Nov 10, 2024

abitrolly commented Nov 11, 2024

bartekpacia commented Nov 11, 2024 •

edited

Loading

bartekpacia commented Nov 23, 2024 •

edited

Loading

abitrolly left a comment

abitrolly Nov 24, 2024

abitrolly commented Nov 24, 2024

bartekpacia commented Nov 24, 2024 •

edited

Loading

abitrolly commented Nov 24, 2024

dearchap Nov 24, 2024

bartekpacia Nov 24, 2024

bartekpacia Nov 24, 2024 •

edited

Loading

dearchap left a comment

abitrolly left a comment •

edited

Loading

bartekpacia commented Nov 24, 2024

Improve the command for printing completion scripts #1998

Improve the command for printing completion scripts #1998

Conversation

bartekpacia commented Oct 28, 2024

Which issue(s) this PR fixes

Special notes for your reviewer

Release Notes

bartekpacia commented Nov 2, 2024

Choose a reason for hiding this comment

bartekpacia Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekpacia Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

abitrolly commented Nov 4, 2024

dearchap commented Nov 4, 2024

dearchap commented Nov 10, 2024

abitrolly commented Nov 11, 2024

bartekpacia commented Nov 11, 2024 • edited Loading

bartekpacia commented Nov 23, 2024 • edited Loading

abitrolly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitrolly commented Nov 24, 2024

bartekpacia commented Nov 24, 2024 • edited Loading

abitrolly commented Nov 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekpacia Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

dearchap left a comment

Choose a reason for hiding this comment

abitrolly left a comment • edited Loading

Choose a reason for hiding this comment

bartekpacia commented Nov 24, 2024

bartekpacia Nov 5, 2024 •

edited

Loading

bartekpacia Nov 24, 2024 •

edited

Loading

bartekpacia commented Nov 11, 2024 •

edited

Loading

bartekpacia commented Nov 23, 2024 •

edited

Loading

bartekpacia commented Nov 24, 2024 •

edited

Loading

bartekpacia Nov 24, 2024 •

edited

Loading

abitrolly left a comment •

edited

Loading