Various completion improvements #140
Conversation
WalkthroughThe recent updates focus on enhancing argument handling and flag completions across various commands, ensuring consistent behavior and user experience. Improvements include setting specific Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (10)
- cmd/bug_report.go (1 hunks)
- cmd/check.go (1 hunks)
- cmd/export.go (1 hunks)
- cmd/import.go (2 hunks)
- cmd/list.go (1 hunks)
- cmd/man.go (1 hunks)
- cmd/remove.go (1 hunks)
- cmd/root.go (2 hunks)
- cmd/update.go (1 hunks)
- cmd/version.go (1 hunks)
Additional comments not posted (13)
cmd/version.go (1)
12-15: The changes to enforce no arguments and disable file completions for theversioncommand are appropriate and align with the PR's objectives.cmd/list.go (1)
15-19: The modifications to enforce no arguments and disable file completions for thelistcommand are well-aligned with the PR's goals.cmd/bug_report.go (1)
13-18: The updates to enforce no arguments and disable file completions for thebug-reportcommand align well with the PR's goals.cmd/root.go (2)
6-7: The addition ofruntimeandstrconvimports supports the new functionality introduced in this PR.
65-71: The implementation ofcompleteNCPUsfor calculating and returning CPU numbers as strings for flag completion is well-aligned with the PR's objectives.cmd/remove.go (1)
24-24: The addition of argument validation to enforce a minimum of one argument for theremovecommand enhances its robustness and user guidance.cmd/import.go (2)
24-25: The updates to enforce no arguments and disable file completions for theimportsubcommand align well with the PR's goals.
34-40: The addition of new arguments and flag completions for specifying file paths and CPU core usage significantly enhances theimportsubcommand's functionality and user experience.cmd/man.go (1)
19-24: The updates to enforce no arguments and disable file completions for themancommand align well with the PR's goals.cmd/export.go (1)
26-27: The modifications to enforce no arguments and disable file completions for theexportcommand are well-aligned with the PR's objectives.cmd/check.go (1)
25-26: SettingArgstocobra.NoArgsandValidArgsFunctiontocobra.NoFileCompletionscorrectly standardizes command behavior for commands that do not require arguments or file completions.cmd/update.go (2)
37-39: Registering the "exclude" flag withcobra.NoFileCompletionsis a good practice, as it guides users towards providing valid input values.
41-43: Consistently applyingcobra.NoFileCompletionsto the "main" flag, as with other flags, helps maintain a uniform user experience.
| if err := cmd.RegisterFlagCompletionFunc("jobs", completeNCPUs); err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Consider handling the error from RegisterFlagCompletionFunc more gracefully instead of panicking. This could improve the robustness of the application by allowing it to handle unexpected situations more gracefully.
- panic(err)
+ print.Err(fmt.Errorf("failed to register flag completion for 'jobs': %w", err))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err := cmd.RegisterFlagCompletionFunc("jobs", completeNCPUs); err != nil { | |
| panic(err) | |
| } | |
| if err := cmd.RegisterFlagCompletionFunc("jobs", completeNCPUs); err != nil { | |
| print.Err(fmt.Errorf("failed to register flag completion for 'jobs': %w", err)) | |
| } |
| if err := cmd.RegisterFlagCompletionFunc("jobs", completeNCPUs); err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
As mentioned in the review of cmd/check.go, consider handling the error from RegisterFlagCompletionFunc more gracefully instead of panicking to enhance application robustness.
- panic(err)
+ print.Err(fmt.Errorf("failed to register flag completion for 'jobs': %w", err))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err := cmd.RegisterFlagCompletionFunc("jobs", completeNCPUs); err != nil { | |
| panic(err) | |
| } | |
| if err := cmd.RegisterFlagCompletionFunc("jobs", completeNCPUs); err != nil { | |
| print.Err(fmt.Errorf("failed to register flag completion for 'jobs': %w", err)) | |
| } |
| // cmd.Flags().BoolP("main-all", "M", false, "update all binaries by @main or @master (delimiter: ',')") | ||
| cmd.Flags().IntP("jobs", "j", runtime.NumCPU(), "Specify the number of CPU cores to use") | ||
| if err := cmd.RegisterFlagCompletionFunc("jobs", completeNCPUs); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
@scop
"gup" is not a library, it's a small-scale code. I don't think "gup" encounters situations where panic() is necessary. It would be preferable to output error messages instead.
There was a problem hiding this comment.
In my opinion, if gup was a library, panic would be completely unacceptable :)
Regarding this case, the only case where registering a flag completion function returns an error is when it is attempted for a flag that does not exist, or for which a completion function is already registered. Both are permanent programming errors on the gup side, and not really expected to happen. Therefore I think panic is appropriate here, and it's good that it happens up front when invoking any gup command, so it's really easily noticeable, no need for any fancier error handling or need to be more "gentle" about it. For ref, see https://github.com/spf13/cobra/blob/6b5f577ebce858ee70fcdd1f062ea3af4b1c03ab/completions.go#L135-L148
If you still disagree, let me know and I can see about doing it some other more elaborate way.
There was a problem hiding this comment.
I wrote a long comment, but the conclusion is "no need for modifications (this code is ok, good job)"
In my opinion, if gup was a library, panic would be completely unacceptable :)
This time, the part where you added panic() is within the new method. If gup were a library, I would tolerate embedding panic() in such places. I use panic() as an assert. That is, when something that I believe should never happen occurs, it indicates (panic()) that the programmer is misusing the method.
However, in the case of a command-line interface, users may not necessarily be developers, so I want to display more understandable error messages. Furthermore, putting panic() makes it harder to test, so I don't like using panic().
However, writing unit tests for initialization code is absurd. Therefore, I would like to adopt your opinion. Reading your thoughtful opinion, I felt that you are an excellent engineer.
See individual commits for details.
Summary by CodeRabbit
New Features
Refactor