-
Notifications
You must be signed in to change notification settings - Fork 434
🐛 fix(console): switch to components->info() #2283
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
base: 1.x
Are you sure you want to change the base?
Conversation
Line now expects at least two arguments in Laravel 12, causing an ArgumentCountError when called with one. Use info to preserve the intended output and restore command compatibility.
WalkthroughUpdated the AddonsDiscover console command to use info() instead of line() for per-addon discovery output. All processing logic, manifest building, iteration flow, final success message, and return value remain unchanged. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/Console/Commands/AddonsDiscover.php (1)
31-33
: Optional: Drop the inner tag or use twoColumnDetail for cleaner formattingIf you don’t specifically need the package name highlighted separately, the inner … is redundant when calling components->info(). Consider simplifying:
- $this->components->info("Discovered Addon: <info>{$package}</info>"); + $this->components->info("Discovered Addon: {$package}");Alternatively, if you want a consistent two-column look with the package name emphasized:
- $this->components->info("Discovered Addon: <info>{$package}</info>"); + $this->components->twoColumnDetail('Discovered Addon', "<info>{$package}</info>");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/core/src/Console/Commands/AddonsDiscover.php
(1 hunks)
🔇 Additional comments (1)
packages/core/src/Console/Commands/AddonsDiscover.php (1)
31-33
: LGTM: Switching to components->info() correctly avoids Laravel 12 ArgumentCountErrorUsing components->info() here aligns with Laravel 12’s updated components API and matches the style already used for the final message on Line 35. Good fix.
@alecritson I don't think we've revisited the addons manifest for a good while. I don't think it's being used. Guessing we left it in for when we did want to re-implement it? |
to be fair i haven't checked the internals/usage of the command, i just ran it and it failed. So i fixed that :) |
components->line requires two arguments in Laravel 12, causing an ArgumentCountError with a single string. Using components->info preserves formatting and restores compatibility. The $this->line helper has a different API and is not interchangeable with components->line.