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
moved this from Ready for Review
to Review in Progress
in Pull Request Progress
m2-community-project
bot
added
Progress: review
and removed
Progress: ready for review
labels
https://wiki.corp.adobe.com/pages/viewpage.action?spaceKey=MC&title=Proposal+to+change+magento-coding-standard+to+not+require+DocBlock+when+parameters+defined