-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
More phpstan fixes #4291
More phpstan fixes #4291
Conversation
# Conflicts: # .phpstan.dist.baseline.neon
Co-authored-by: Ng Kiat Siong <[email protected]>
@sreichel - The conflict needs to be fixed. |
# Conflicts: # .phpstan.dist.baseline.neon # app/code/core/Mage/Adminhtml/Model/Sales/Order/Create.php # app/code/core/Mage/Tax/Model/Resource/Calculation.php
I think these massive PRs should be accepted only if they don't add new lines to the baseline. we know nobody is testing these modifications and there are hundreds of changed files every day. not having new lines is at least a basic indicator that maybe the PR itself is not introducing problems. |
@fballiano i alredy wrote it somewhere else ... fixing one error may reveal more errors. Fixing one may lead to have to touch other files. There are no problems introduced, just baselined them to them later. #4346 is to fix that 4 new lines added in this PR + some issues in that files. |
Instead of changing 100 files per day, if a change is introducing new baseline lines than it should be in a separate small PRs. Changing 100+ files per day means nobody is testing anything. |
Normally no new lines are added! Normally there is nothing to test, b/c its mostly doc-block-only changes. I dont know what you are complaining about. 4 (!) errors have been added - in trade for 150 fixed. |
* some fixes - #4291 (comment) * some fixes * rector * Update app/code/core/Mage/Paypal/Model/Payflowlink.php Co-authored-by: Ng Kiat Siong <[email protected]> --------- Co-authored-by: Ng Kiat Siong <[email protected]>
Some fixes and merged #4290