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
Improve the command for printing completion scripts #1998
Changes from 15 commits
fc30c1e
ed434ac
bf5d792
717b07c
f554abb
f81b2cb
4dcd89b
c84b5ca
cffef65
24524da
319a061
cc5c66a
d211484
e27feb4
9b41c34
de30973
afd23df
4f29af7
19c9c1e
4f2cec4
c5ff653
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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?
Check warning on line 28 in completion.go
Codecov / codecov/patch
completion.go#L27-L28
Check warning on line 35 in completion.go
Codecov / codecov/patch
completion.go#L34-L35
Check warning on line 45 in completion.go
Codecov / codecov/patch
completion.go#L44-L45
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.
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 bikesheddingexamples/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:
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?