-
Notifications
You must be signed in to change notification settings - Fork 109
fix: [#850] artisan help incorrect behavior #1329
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
Conversation
Summary of ChangesHello @hwbrzzl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue with the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request addresses an issue with the artisan help command's behavior. The changes correctly replace the help command with a list command, which is now the default when artisan is run without arguments. The HelpCommand has been removed, and its functionality of displaying the list of all commands has been consolidated into the ListCommand, which now directly calls cli.ShowAppHelp. This refactoring also improves the code by removing a dependency from ListCommand on the Artisan interface. The tests have been updated accordingly to reflect these changes. The overall implementation is clean, correct, and improves the codebase.
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.
Pull request overview
This PR fixes an issue with the artisan help command's incorrect behavior by consolidating the help and list commands into a single list command and removing a circular dependency.
- Removed the separate
HelpCommandand its registration - Updated
ListCommandto directly callcli.ShowAppHelp()instead of usingartisan.Call("--help") - Changed the default artisan command (when no arguments provided) from
helptolist
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| console/console/help_command.go | Deleted the redundant HelpCommand implementation |
| console/console/list_command.go | Simplified ListCommand by removing artisan dependency and directly using cli.ShowAppHelp() |
| console/service_provider.go | Removed HelpCommand registration and updated ListCommand constructor call |
| console/application.go | Changed default command from "help" to "list" when artisan is invoked without arguments |
| console/cli_helper_test.go | Updated tests to use "list" command instead of "help" command |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v1.16.x #1329 +/- ##
==========================================
Coverage ? 68.04%
==========================================
Files ? 215
Lines ? 11601
Branches ? 0
==========================================
Hits ? 7894
Misses ? 3327
Partials ? 380 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
console/console/help_command.go
Outdated
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.
The help command should be added incorrectly, the list command should be used directly. And the help command is not exposed to users in the document, so it's safe to remove it directly, it's duplicated with the list command.
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.
Actually, they are not redundant, so directly removing the help command is not appropriate. The help command can display help information for a specific command, such as ./artisan help build (which is equivalent to ./artisan build --help), and so on. Laravel also provides this feature, and we also supported this previously.
|
By the way, I noticed that the implementation in framework/console/application.go Lines 64 to 66 in 6355289
won’t actually be executed, because the --help flag is handled before the cli action method is called.
|
|
Alternatively, this PR could just focus on fixing the bug with the |
The code was optimized in #1243, the global help is disabled due to the concurrent issue.
|
|
Yes, it's fine to add the new command |
I mean this piece of code: if cliCtx.OptionBool("help") {
return cli.ShowCommandHelp(ctx, cmd, cmd.Name)
}won’t execute. You can test it yourself. Also, I didn’t say --help isn’t working—it does work, but that’s solely because the cli package internally handles this flag separately. |
This |
|
@almas-x
|
It won’t execute. Add a print statement before this if to check if it gets printed. |



📑 Description
Closes goravel/goravel#850
✅ Checks