Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
Improve the command for printing completion scripts #1998
Changes from 10 commits
fc30c1e
ed434ac
bf5d792
717b07c
f554abb
f81b2cb
4dcd89b
c84b5ca
cffef65
24524da
319a061
cc5c66a
d211484
e27feb4
9b41c34
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
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.
can you provide the reason why you moved away from embedFs to having the completion inline with the code ? In the previous approach it was very easy to lookup where the autcompletes were and what they were doing
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.
This is to make it easier to use
%[1]s
since then declaration and usage is in the same place.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'm not sure about this either. We get nice syntax highlighting from the code editor/IDE when the autocomplete is in a separate file. But when it's inlined, it's treated as a string, which makes it less readable.
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 wouldn't say it's a big problem. Characters like
%[1]
aren't valid shell function names anyway and will probably be highlighted wrongly.I also think keeping everything related to completion in a single file is quote convenient.
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.
Scrolling back and forth between template and its code doesn't seem convenient to me. It is better to have two panes - one with template and another with source code that prepares data for it.
Highlighting of template variables as errors could actually be a feature.
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
orexample-hello-world
and add a few subcommand andEnableShellCompletion: 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.