-
Notifications
You must be signed in to change notification settings - Fork 165
Member variables and methods should not require DocBlock when properly typed #476
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
...y typed https://developer.adobe.com/commerce/php/coding-standards/docblock/ > Include all necessary information without duplication. Example 1: ``` private ?ObjectManagerInterface $objectManager; ``` should not require a comment ``` /** @var ObjectManagerInterface|null $objectManager */ ``` because all of that information is duplicated. (Though the nullable is actually harder to read in DocBlock standard.) Example 2: ``` public function getWeakMap() : WeakMap { return $this->weakMap; } ``` This method is expressive. It is obvious what it does. It is strictly typed. There is no reason that it should need a DocBlock. This change no longer requires DocBlock in these cases: 1: member variables that have defined types 2: methods where all parameters have defined types AND method has defined return type (__construct and __destruct won't require return type, since it isn't allowed)
Adding in code to check the namespace of the class/interfaces to see if it is API. Magento's API code requires method Doc Blocks because it doesn't use Reflection for types. https://developer.adobe.com/commerce/php/development/components/web-api/services/
@JacobBrownAustin thanks for raising this. Please can you add some tests to cover the changes being introduced here. I don't know why the automated test runs failed in GitHub Actions; do the tests run successfully on your system?
Yeah; I'm working on updating tests now.
This URL doesn't work for me. Is this an internal reference within Adobe? What does this page say?
Yeah I'll update the details if this to include the info from that page. Basically, I proposed that we shouldn't need superfluous DockBlock for property or method if the type is already defined. It's also solving a separate issue I found more recently that readonly properties fail even when they have proper DockBlock.
There we're a lot of changes in PHP_CodeSniffer v3.8.0 related to readonly. It's possible that the bug you mention in passing here has already been solved. Perhaps you could open an issue here so that can be discussed/investigated.
See also #406
* Updated unit tests to cover new functionality reguarding when we can safely skip requiring DockBlocks * Fix MethodArgumentsSniff to work on PHP files without namespaces
aa44722 to
1fa5365
Compare
1fa5365 to
59b5d46
Compare
...tandard into parameters-defined-needs-no-docblock-except-complex
@fredden , I just noticed that this PR is still open. Is there any changes that you want me to make?
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 looks good to me. It would be good to get some more test coverage.
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 method has no $text parameter. If this is an intentional test, please add a comment to show the same.
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.
Please add a test to cover this case - where a docblock is required due to high complexity score.
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.
Please add a test to cover this case - where a docblock is required due to a class being an API.
https://wiki.corp.adobe.com/pages/viewpage.action?spaceKey=MC&title=Proposal+to+change+magento-coding-standard+to+not+require+DocBlock+when+parameters+defined