-
Notifications
You must be signed in to change notification settings - Fork 109
feat: migrate PHPStan 1 => 2 #393
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
WalkthroughBumps many runtime and dev composer dependencies and adds new geocoder providers; updates PHPStan configuration and baseline (including new ignore identifiers); and makes two small provider/plugin code changes in Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🔇 Additional comments (5)
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. Comment |
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)
composer.json (1)
82-82: Good addition for developer convenience.The phpstan script provides a convenient way to run PHPStan with an appropriate memory limit. The script will automatically use the configuration file (phpstan.neon or phpstan.neon.dist) if present in the project root.
Optionally, you could make the script more explicit by specifying the configuration file and analyze paths:
Optional: Make the script more explicit
- "phpstan": "vendor/bin/phpstan --memory-limit=1024m" + "phpstan": "vendor/bin/phpstan analyze --memory-limit=1024m --configuration=phpstan.neon"This would make it clearer what's being analyzed and which configuration is used, though the current version is perfectly functional.
|
You can move to baseline them |
|
Btw i think we should bump more deps:
|
|
This change needs a closer look https://github.com/geocoder-php/BazingaGeocoderBundle/pull/393/files#diff-2bddfd9704c87a4483aa410e32c197e9f27261146f5226930bbe92c878255552R57 As we bumped the HereFactory, i guess the change should not change anythink in userland. |
|
Thanks @Chris53897 |
The goal is to improve code quality further.
I am not sure if i can fix all of the errors.
And have the time
Summary by CodeRabbit
Chores
New Integrations
Bug Fixes / Behavior
✏️ Tip: You can customize this high-level summary in your review settings.