-
Notifications
You must be signed in to change notification settings - Fork 50
Add ShouldCallParentMethodsRule #79
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
e448b0b to
fea805d
Compare
fea805d to
0cefcd9
Compare
rules.neon
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.
Instead of adding this as a new rule which would block another 0.12.x release, could you:
- Require PHPStan 0.12.33 as it contains the bleedingEdge parameter
- Add this as a conditional rule like this:
conditionalTags:
PHPStan\Rules\PHPUnit\ShouldCallParentMethodsRule:
phpstan.rules.rule: %featureToggles.bleedingEdge%
services:
-
class: PHPStan\Rules\PHPUnit\ShouldCallParentMethodsRule
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.
Once we have it in conditionalTags, it should no longer be in the rules section.
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.
This isn't necessary, PHPStan already knows the type.
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.
This would accept parent::setUp() in tearDown method, and vice-versa. We need to be more strict.
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.
I'd like 'Missing call to parent::setUp() method.'. more
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.
Almost done :)
rules.neon
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.
Once we have it in conditionalTags, it should no longer be in the rules section.
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.
This should be formatted the same way as the next item (open square bracket on the next line)
Thank you, I'll tag it once I have a computer again :)
Tagged as 0.12.14 :)
Great! Thanks.
FYI found out very quickly the logic wasn't quite right, just fixed it :) 264abf4
Closes #78