-
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
base: main
Are you sure you want to change the base?
Changes from all commits
fc30c1e
ed434ac
bf5d792
717b07c
f554abb
f81b2cb
4dcd89b
c84b5ca
cffef65
24524da
319a061
cc5c66a
d211484
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
package main | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 Maybe I can modify There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 I agree with @abitrolly comment that it's be nice to have a single place for more "full app" examples. |
||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"log" | ||
"os" | ||
|
||
"github.com/urfave/cli/v3" | ||
) | ||
|
||
func main() { | ||
app := &cli.Command{ | ||
Name: "simpletask", | ||
Usage: "a dead simple task manager", | ||
EnableShellCompletion: true, | ||
Action: func(ctx context.Context, command *cli.Command) error { | ||
fmt.Println("decide what to do!") | ||
return nil | ||
}, | ||
Commands: []*cli.Command{ | ||
{ | ||
Name: "add", | ||
Aliases: []string{"a"}, | ||
Usage: "add a task to the list", | ||
Action: func(ctx context.Context, cmd *cli.Command) error { | ||
fmt.Println("added task: ", cmd.Args().First()) | ||
return nil | ||
}, | ||
}, | ||
{ | ||
Name: "complete", | ||
Aliases: []string{"c"}, | ||
Usage: "complete a task on the list", | ||
Action: func(ctx context.Context, cmd *cli.Command) error { | ||
fmt.Println("completed task: ", cmd.Args().First()) | ||
return nil | ||
}, | ||
}, | ||
{ | ||
Name: "template", | ||
Aliases: []string{"t"}, | ||
Usage: "options for task templates", | ||
Commands: []*cli.Command{ | ||
{ | ||
Name: "add", | ||
Usage: "add a new template", | ||
Action: func(ctx context.Context, cmd *cli.Command) error { | ||
fmt.Println("new task template: ", cmd.Args().First()) | ||
return nil | ||
}, | ||
}, | ||
{ | ||
Name: "remove", | ||
Usage: "remove an existing template", | ||
Action: func(ctx context.Context, cmd *cli.Command) error { | ||
fmt.Println("removed task template: ", cmd.Args().First()) | ||
return nil | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
err := app.Run(context.Background(), os.Args) | ||
if err != nil { | ||
log.Fatal(err) | ||
} | ||
} |
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.