-
Notifications
You must be signed in to change notification settings - Fork 101
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
Update for Cake 5. #836
Update for Cake 5. #836
Conversation
859320d
to
da49353
Compare
@othercorey Slevomat CS is barfing on EDIT: Figured it out. It blew up because a docblock had |
d6cbe1a
to
df53683
Compare
phpstan decided to change the return type which broke slevomat. |
src/BakePlugin.php
Outdated
use DirectoryIterator; | ||
use ReflectionClass; | ||
use ReflectionException; | ||
|
||
/** | ||
* Plugin class for bake | ||
*/ | ||
class Plugin extends BasePlugin | ||
class BakePlugin extends BasePlugin | ||
{ | ||
/** | ||
* Plugin name. | ||
* | ||
* @var string |
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.
* @var string | |
* @var string|null |
if you meant for this to be nullable
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 wish we didn't have to add these redundant docblock types anymore :(.
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.
We need to do a test with the slevomat setting that detects redundant blocks. I don't know if it forces all @param annotations to be removed even if there's a description you want to keep. Should we just not have comments on parameters annotations anymore?
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 don't think we should drop parameter comments but I doubt it would allow param description without 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.
Need to test if a comment prevents it from being marked useless.
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 would rather check why the sniffs are missing for it:
https://github.com/spryker/code-sniffer/blob/master/Spryker/Sniffs/Commenting/DocBlockReturnNullSniff.php
or
https://github.com/spryker/code-sniffer/blob/master/Spryker/Sniffs/Commenting/DocBlockReturnNullableTypeSniff.php
could be the right direction here.
As long as they are properly synced, which is easier, all is well.
looks good. |
No description provided.