Member variables and methods should not require DocBlock when properly typed#476
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 |
|
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
JacobBrownAustin
force-pushed
the
parameters-defined-needs-no-docblock-except-complex
branch
2 times, most recently
from
aa44722 to
1fa5365
Compare
JacobBrownAustin
force-pushed
the
parameters-defined-needs-no-docblock-except-complex
branch
from
1fa5365 to
59b5d46
Compare
m2-community-project
bot
added
Progress: review
and removed
Progress: ready for review
labels
…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? |
fredden
left a comment
fredden
left a comment
There was a problem hiding this comment.
This looks good to me. It would be good to get some more test coverage.
| * @param string $text | ||
| * @return string | ||
| */ | ||
| #[\ReturnTypeWillChange] | ||
| public function methodWithAttributeAndValidDocblockWithoutTypes() |
There was a problem hiding this comment.
This method has no $text parameter. If this is an intentional test, please add a comment to show the same.
| } | ||
| } | ||
| $complexity = $this->getMethodComplexity($phpcsFile, $stackPtr); | ||
| return $complexity > static::MAXIMUM_COMPLEXITY_ALLOWED_FOR_NO_DOCBLOCK; |
There was a problem hiding this comment.
Please add a test to cover this case - where a docblock is required due to high complexity score.
| if ($this->checkIfNamespaceContainsApi($phpcsFile)) { | ||
| // Note: API classes MUST have DocBlock because it uses them instead of reflection for types | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Please add a test to cover this case - where a docblock is required due to a class being an API.
2 participants
https://wiki.corp.adobe.com/pages/viewpage.action?spaceKey=MC&title=Proposal+to+change+magento-coding-standard+to+not+require+DocBlock+when+parameters+defined