Remove rules duplicating method signature info #4425
Conversation
|
An admin must run tests on this PR before it can be merged. |
Vinai
changed the title
|
Please hold this PR until after it was discussed in an architecture meeting. |
|
@buskamuza this PR is about removing ALL THE useless stuff.
Please provide some reasoning. |
|
@orlangur She did in the Slack thread - I added that to the PR:
|
| If there is no such operator, the `@return` tag must have `void` as the return value. | ||
| * The declaration of all arguments (if any) using `@param` tag, unless the argument type is indicated in the method signature. | ||
| All `@param` annotations must include the appropriate argument type. | ||
| If any argument requires a `@param` annotation, all arguments must be listed (all or none). |
There was a problem hiding this comment.
Disagree. Let's specify @param only for necessary arguments, when it is more accurate than type hinting.
There was a problem hiding this comment.
In my experience incomplete lists of params in the annotations lead to less readable code.
There was a problem hiding this comment.
Some examples? I don't think @param annotations are for humans at all, they are to give maximum accurate information for IDE. Besides constructor, any method don't contain more than 4-5 parameters usually, so, I believe such block with partial types for arguments missing real type hinting will be readable anyway.
There was a problem hiding this comment.
I know default PhpStorm will complain about partial doc blocks. We could change this, but a lot of developers will not understand it and it will lead to inconsistencies. Not sure how IDEs or PhpDoc will handle the order of the arguments then.
There was a problem hiding this comment.
I don't think there is an absolute truth to be reached in this regard (as is often the case in regards to "understandability". Who can decide this matter?
There was a problem hiding this comment.
Can we also add a rule that @param annotations should be in the same order as method arguments?
There was a problem hiding this comment.
In my experience incomplete lists of params in the annotations lead to less readable code.
Do you have some examples? There are not a lot of situations when we will need annotation for parameter at all (only more accurate type like array -> Product[] and such). Methods beside constructor usually won't contain more than 3-4 arguments.
There was a problem hiding this comment.
I don't have any examples at hand, since I changed those to contain all parameters instead.
But here is a constructed example:
Single @param annotation:
/**
* @param SectionSourceInterface[] $sectionSourceMap
*/
public function __construct(
ObjectManagerInterface $objectManager,
SectionIdentifier $identifier,
array $sectionSourceMap = []
) {
$this->objectManager = $objectManager;
$this->identifier = $identifier;
$this->sectionSourceMap = $sectionSourceMap;
}A complete list of parameters annotations:
/**
* @param ObjectManagerInterface $objectManager
* @param SectionIdentifier $identifier
* @param SectionSourceInterface[] $sectionSourceMap
*/
public function __construct(
ObjectManagerInterface $objectManager,
SectionIdentifier $identifier,
array $sectionSourceMap = []
) {
$this->objectManager = $objectManager;
$this->identifier = $identifier;
$this->sectionSourceMap = $sectionSourceMap;
}|
|
||
| * Use `@inheritdoc` (notice no braces around) to indicate that the entire doc block should be inherited from the parent method. | ||
| * Use the inline `{@inheritdoc}` tag (with braces around) in long descriptions to reuse the parent's long description. The tagged method MUST have its own short description. | ||
| The `@inheritdoc` tag SHOULD NOT be used. |
There was a problem hiding this comment.
I'll leave this open while the discussion is ongoing.
| * Use `@inheritdoc` (notice no braces around) to indicate that the entire doc block should be inherited from the parent method. | ||
| * Use the inline `{@inheritdoc}` tag (with braces around) in long descriptions to reuse the parent's long description. The tagged method MUST have its own short description. | ||
| The `@inheritdoc` tag SHOULD NOT be used. | ||
| If a child class method requires a long description to explain its purpose, it may use `@inheritdoc` to indicate the new description is intended as an addition to the parent method description. |
There was a problem hiding this comment.
We may allow {@inheritdoc} inline version only but I believe it would be better to forbid completely.
There was a problem hiding this comment.
Agreed. I don't think I've ever seen someone using inline inheritdoc without it looking weird in the documentation.
There was a problem hiding this comment.
What's the current state in regards to @inheritdoc? I'm all for getting rid of it, but if @buskamuza wants to allow it I'm okay with that, too, as long as it's discouraged to use it situations where it is only noise.
There was a problem hiding this comment.
I agree with keeping just "The @inheritdoc tag SHOULD NOT be used." if it's ok for everyone.
I don't see why it should be "MUST NOT", as it doesn't break anything. But I'm ok with discouraging.
There was a problem hiding this comment.
@Vinai additional notes to untouched yet lines.
"Include all necessary information." -> "Include all necessary information without its duplication."
"So in general case, classes that are declared in dedicated files, must have one DocBlock, which refers to the class and file at the same time." -> now DocBlock is not manmdatory, must -> must have one or no Doc Block
protected $_logger; please change to priavte $logger;
* @param Random $mathRandom
* @param StdlibDateTime $dateTime
* @param int $number
let's make no unnecessary alignment mandatory
Let's make
/** @var Profiler */
the only form allowed.
@category, @package, and @subpackage MUST NOT be used please add @author also.
So, we can just remove the "In a given DocBlock, style of formatting must be consistent." section in favor of "don't align anything:" form.
|
@Vinai also, I'll double check regarding v2.1 and v2.2 in terms of used sniffs but my gut feeling says that we should only revise v2.3 here (I would be able to apply automated PHPDoc change for all the three release lines though). |
|
@orlangur I used the 2.1 version of the page because 2.2 and 2.3 just referenced it. |
|
Ok, let me check automated checks in 2.1/2.2 first and then will ask somebody from DevDocs team. |
| @@ -364,7 +368,7 @@ Functions and methods must have: | |||
| For example: `@return Config|null`. | |||
| The DockBlock needs to explain what situations return `null`. | |||
|
|
|||
| Another example: `@param FileInterface | null`. | |||
| Another example: `@param FileInterface|null`. | |||
There was a problem hiding this comment.
Any reason you didn't opt for the nullable operator? @param ?FileInterface
There was a problem hiding this comment.
I find it strange to use nullable on @param annotations. Is that a good idea? I honestly don't know. So far I've never encountered that in annotations. Happy to change it if you think it's good and PHPStorm understands it.
|
@orlangur I've applied most changes you requested in #4425 (review) However:
and
I'm against those changes - they give me the feeling as if I'm beeing micro managed by someone oppressive. |
|
@Vinai thanks, no problem, to me those make sense if they are supported by auto-formatting tooling. Will try to address them separately. Asked Olga in Slack regarding inheritdoc and will double-check the overall document in next few days. Huge thanks on your efforts to aggregate all the pain points related to redundancy 👍 |
|
@buskamuza Your requested change to allow |
| If there is no such operator, the `@return` tag must have `void` as the return value. | ||
| * The declaration of all arguments (if any) using `@param` tag, unless the argument type is indicated in the method signature. | ||
| All `@param` annotations must include the appropriate argument type. | ||
| If any argument requires a `@param` annotation, all arguments must be listed (all or none). |
There was a problem hiding this comment.
In my experience incomplete lists of params in the annotations lead to less readable code.
Do you have some examples? There are not a lot of situations when we will need annotation for parameter at all (only more accurate type like array -> Product[] and such). Methods beside constructor usually won't contain more than 3-4 arguments.
|
@orlangur I'm happy to do whatever in regards to |
| @@ -425,7 +430,7 @@ In general, use the `@throws` tag when the code uses *throw*: | |||
| * | |||
| * @param string $elementId | |||
| * @param string $attribute | |||
| * @param mixed $value | |||
| * @param int|string|float|bool|object|null $value | |||
There was a problem hiding this comment.
Why is this change needed? It is not exactly equivalent transformation since mixed is broader.
There was a problem hiding this comment.
This was a request by @orlangur - I personally think mixed is fine, but don't feel strongly either way.
There was a problem hiding this comment.
@paliarush do you mean it can be resource? Generally mixed does not smell well.
There was a problem hiding this comment.
Besides resource, the mixed type annotation includes any future types, too.
|
@Vinai @buskamuza in which case we need @Vinai another thing we discussed earlier - could you provide example when partial parameters documentation may be confusing? We will need parameter type in PHPDoc only in rare cases, like a typified array |
|
@orlangur Regarding partial vs. complete parameter annotations: I believe it boils down to familiarity which scenaerio is more or less confusing. We will not reach an agreement with examples here. The use cases that require type annotations beyond what PHP is capable of typing are:
Just saying, because in my experience they aren't such a rare ocurance... |
There was a problem hiding this comment.
Thanks @Vinai for your endeavors on combining this altogether. My current bunch of comments is complete for the changed rows, please review them.
I didn't read a new version of document as a whole, let's update it with this PR squashed and then it can be simplified further in subsequent PRs.
| All `@param` annotations must include the appropriate argument type. | ||
| If any argument requires a `@param` annotation, all arguments must be listed (all or none). | ||
| * The `@param` annotations must be in the same order as the method arguments. | ||
| * The declaration of the return type using the `@return` tag must only be added if the method return type signature |
There was a problem hiding this comment.
It is unclear whether @return void is still mandatory or not?
There was a problem hiding this comment.
Does the sentence exclude voidin some way? To me it seems clear.
Should void be made explicit?
e.g: If a method return type signature is void then the @return void annotation must be omitted.
| * Constructors may not have short and/or long description | ||
|
|
||
| * Testing methods in Unit tests may not have doc blocks if the test's method name follows the convention (test<MethodName>) | ||
| * Testing methods in Unit tests may have doc blocks to describe the purpose of the test, for example referencing github issues. |
There was a problem hiding this comment.
Honestly, I like previous version more. My understanding is that we should insist descriptive enough test<MethodName> method name without any PHPDoc is preferred.
There was a problem hiding this comment.
A test name with a class name often is not sufficient. Since tests are documentation, I think it's good to be more permissive than the previous version of the sentence.
|
|
||
| * If the test does not follow the convention, it should have a doc block describing the covered methods | ||
| * Test method annotations may include data providers and other testing annotations. |
There was a problem hiding this comment.
Because it is implicit?
| @@ -492,21 +497,24 @@ public function deleteDirectory($path) | |||
| #### @return tag | |||
| {:#return} | |||
|
|
|||
| If there is no explicit return statement in a method or function, a `@return void` should be used in the documentation. | |||
| In general method return type signatures should be prefered over `@return` type annotations. | |||
There was a problem hiding this comment.
I would say
Return type signature MUST be used whenever possible.
`@return` annotation MUST be omitted when it brings no new information compared to type signature.
There was a problem hiding this comment.
Restricting types make code harder to evolve over time. I believe it is better to sometimes NOT specify a return type until a method signature is very stable. This more permissive statement allows for this.
@Vinai sure, but it won't happen for too many parameters of the same method. And such accurate type specification is more for IDE type resolution than human reading. |
dobooth
changed the title
|
Moved all changes to |
|
running tests |
|
Hi @Vinai, thank you for your contribution! |
|
@Vinai glad to finally see it in, you rock 🎉 Special thanks for squashing prior to merge 😉 Let's start the biggest cleanup ever made for core codebase. |
Remove rules that duplicate information in method signatures.
Purpose of this pull request
This pull request (PR) removes the rules enforcing useless phpdoc annotations.
These useless annotations are generally not liked and add no information beyond what already is supplied by the class, interface or method name and the method signature.
Affected DevDocs pages