-
Notifications
You must be signed in to change notification settings - Fork 9
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
chore: improve static analysis, bump min PHP to 7.4, support PHP 8.2 #31
base: main
Are you sure you want to change the base?
Conversation
chore: add linting and fix for style guide
@ramsey my two cents, #26 should go first given it's been open for a really long time and will get even more delayed if there are merge conflicts. Also, +1 to @Makapashev's comment. |
@ramsey You may add code coverage baseline to control that it is not downgraded with new PRs :) |
@Aeliot-Tm would you consider such code coverage baseline a blocker for getting this PR in a mergeable state? or could it be handled through a follow-up? |
@ramsey any reason why both PHPStan and Psalm as static code analysis tools are added? Yes they can capture marginally different things, but solely starting with PHPStan with a baseline file might be a more controllable way of adding static analysis (e.g. as all future contributors will also have to work with it)? |
Proposed changes
Since this adds parameter types and return types in many places where there were none, this can break BC for applications using this and extending classes or passing the wrong values into methods. As such, I recommend bumping the package's major version when doing the next release (after this is merged).
🚨 This stands to cause a lot of merge conflicts with #26. I'm not sure how we want to handle that. Which should we merge first?
How to test
composer test
Unit Tests