-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Improve the command for printing completion scripts #1998
Improve the command for printing completion scripts #1998
Conversation
4c63582
to
ed55e57
Compare
I'm having trouble understanding why it fails. Running Will appreciate help here @dearchap @meatballhat @abitrolly |
ed55e57
to
24524da
Compare
examples/simpletask/main.go
Outdated
@@ -0,0 +1,70 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats where the https://github.com/urfave/cli/blob/ef45965eeb9c1122885fafa4a391b6be6a674f3d/examples_test.go comes into play.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dearchap https://github.com/urfave/cli/blob/ef45965eeb9c1122885fafa4a391b6be6a674f3d/examples_test.go is not discoverable, and also they do not work if copy-pasted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I agree. I will add the simpletask
example in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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? |
@abitrolly All the template code is already in current binary. So @bartekpacia 's work doesnt really change. |
Co-authored-by: dearchap <dear.chap@gmail.com>
This reverts commit cffef65.
2da1d53
to
8a0b42d
Compare
8a0b42d
to
9b41c34
Compare
@Juneezee @dearchap @abitrolly i moved completion script strings back into individual files. I think this PR is good to be merged now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
examples/simpletask/main.go
Outdated
@@ -0,0 +1,70 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
b901b8d
to
4f29af7
Compare
I am not aware how this feature works. Is it enabled by default? |
This PR does 2 things:
No other behavior changes are expected. |
Maybe make 2 PRs making 1 thing, so that we can finally merge at least something already. :D |
"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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if we can ignore the error since we are loading from embedFS. Saves us from writing unit tests as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch! I think it's not worth worrying about the error here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just decided to not additionaly wrap the errors to minimize amount of changes.
I think it's fine now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but the code cov decreased, I've worked so much over the last month to get the code coverage from 80 something percent to 98.4% and now it decreases again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't think about it. I'm sorry!
What command can I run locally to see coverage percentage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to add additional tests or remove err in the lookup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
thanks for the review everyone :) I will of course "squash and merge" this PR to not butcher history |
|
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 simplycompletion
. 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:
To enable completions permanently by placing the completion scripts into the standard completions directory:
Please note that for completion to work, your top-level
cli.Command
name and your binary name must be the same.