From the API documentation: "Every function, constant, class, interface, class member (function, property, constant), and file must be documented, even private class members.". I think this should be changed so it is optional to place comments. Because of this requirement most comments are absolutely useless and just present because they need to be. To give some examples:
/**
* The renderer service.
*
* @var \Drupal\Core\Render\Renderer
*/
protected $renderer;
/**
* The HTML response attachments processor service.
*
* @var \Drupal\Core\Render\AttachmentsResponseProcessorInterface
*/
protected $htmlResponseAttachmentsProcessor;
Why does this need to have the comment "The renderer service."? From both the name of the variable and the @var it is already very clear that it's a renderer service. Code should be self explaining and adding comments for the sake of requiring comments just makes it more bulky and less readable because you are able to see less lines of actual code on your screen. In this case the previous example could be written as:
/**
* @var \Drupal\Core\Render\Renderer
*/
protected $renderer;
/**
* @var \Drupal\Core\Render\AttachmentsResponseProcessorInterface
*/
protected $htmlResponseAttachmentsProcessor;
This already saves 4 lines of comments while no context is lost about what these methods are. To give another example on a construct of a class:
/**
* Constructs a new HttpKernel decorator.
*
* @param \Symfony\Component\HttpKernel\HttpKernelInterface $httpKernel
* Original http kernel.
* @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
* Entity Type Manager.
* @param \Drupal\Core\PageCache\ResponsePolicy\KillSwitch $killSwitch
* Kill switch service.
* @param \Drupal\Core\Password\PasswordInterface
* Password checker service.
*/
public function __construct(HttpKernelInterface $httpKernel, EntityTypeManagerInterface $entityTypeManager, KillSwitch $killSwitch, PasswordInterface $passwordChecker) {
$this->httpKernel = $httpKernel;
$this->entityTypeManager = $entityTypeManager;
$this->killSwitch = $killSwitch;
$this->passwordChecker = $passwordChecker;
}
could look like:
/**
* @param \Symfony\Component\HttpKernel\HttpKernelInterface $httpKernel
* @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
* @param \Drupal\Core\PageCache\ResponsePolicy\KillSwitch $killSwitch
* @param \Drupal\Core\Password\PasswordInterface
*/
public function __construct(HttpKernelInterface $httpKernel, EntityTypeManagerInterface $entityTypeManager, KillSwitch $killSwitch, PasswordInterface $passwordChecker) {
$this->httpKernel = $httpKernel;
$this->entityTypeManager = $entityTypeManager;
$this->killSwitch = $killSwitch;
$this->passwordChecker = $passwordChecker;
}
In this case all required comments describing the function and variables are just a waste of space and make the code less readable.
TLDR; Good code doesn't need comments and is self explaining by the use of @param, @var tags etc, don't require developers to put useless comments everywhere.
Comments
Comment #2
euphoric_mv CreditAttribution: euphoric_mv commented+1 I totally agree that descriptions in DocBlocks should be optional.
Comment #3
drunken monkey-1, I like this as consistent as possible. Otherwise, we'll just have people arguing over which members are self-explanatory and which need comments. And it would be harder to spot where doc blocks were actually forgotten.
Also: “Good code doesn't need comments” is definitely wrong, but I guess you didn't mean it like that.
Comment #4
slootjes CreditAttribution: slootjes commented@drunken_monkey #3 I do mean exactly that. When using proper names of classes, methods and variables it should be pretty much self-explaining. Please explain me how it helps to describe a property 3 (!) times:
- The name of the property should be descriptive
- The @param/@var in the doc block for auto completion in the IDE
- The description
What is useful about the 3rd one? When I look at core 99/100 times this is simply "The {Name Of The Class} Service."....why is this helpful?
The example clearly shows that you can remove 6 lines of code (comment actually) so we can fit more useful code on the screen and don't have to worry about forgetting to put down a bunch of useless comments. We're developers and we should be able to figure out what the propery is capable of when reading the name + doc block declaration without a required description.
Comment #5
pfrenssen-1 One of the main strengths of Drupal is its awesome documentation.
First of all that's not true, the majority of classes are properly documented. If you find one that uses this pattern then please file a documentation issue so that it can be fixed.
Comment #6
slootjes CreditAttribution: slootjes commented@pfrenssen When doing a search on "Constructs a" in core/lib PhpStorm finds "100+ matches in 98+ files". Also core is filled with "The something service.". You can do the searches yourself easily. Documentation is good if it adds something but documentation like that is purely added because it's required, not because it actually adds something.
Comment #7
pfrenssen@slootjes yes I understand that you don't like writing the same documentation three times in a single document. Nobody likes to do that. But you have to look at the side of the people that have to work with your code.
You as a developer only needs to write it once but then over its lifetime your documentation will be read thousands of times by hundreds of developers who are not familiar with your code, or who do not know what this particular interface represents. It is also parsed to use in online API documentation, and IDEs can use this information to show in tooltips and when doing autocompletion.
Comment #8
slootjes CreditAttribution: slootjes commentedIf the comments would actually make sense in adding something that can't be read by code I completely agree but adding "The something service.", explain me what this adds to the readability of the code? If the interface needs further information then obviously comments should be added but in most cases the name of it is sufficient and the comments are more useful in the actual interface, not the class implementing it.
Again, I agree with this to a certain level when comments add something usefull but there are so many sily comments in core that are just clutter. Besides, and this sounds really harsh, but if someone is looking at __construct() of FooBar class and doesn't understand that it will return an instance of FooBar what does the comments "Construct a FooBar instance." add to the documentation which already shows the name of the class, the function __construct and it's possible arguments. For factories this could actually be helpful, that's why I would like to see it being optional instead of mandatory. Don't let developers write comments for the sake of adding useless comments, let them spend their time developing.
What should it be changed to instead then to make it right again so it does add value?
Comment #9
pfrenssen@slootjes I understand your point, it's not necessary to repeat the same argument again and again.
I think this is quite telling where your understanding goes wrong about why we have this documentation:
The point of documentation is to make sure you don't have to read the code. Drupal developers are humans, not PHP parsing automatons. When scanning through a code file it is much easier and faster for a human to read "The bundle info service" than it is to read
@param \Drupal\Core\Entity\EntityTypeBundleInfoInterface $entity_type_bundle_info
.I can bet that you don't mind reading this documentation at all. You just don't like writing it. That's fine, nobody likes it. I don't either. But that's not very important, we have so many developers that are involved with Drupal every day that every line of Drupal code is "write once, read 10000 times".
Comment #10
slootjes CreditAttribution: slootjes commentedMaybe this is where we disagree then, I prefer reading the code instead.
I actually do mind seeing it because as a result I can fit less code on my screen. And again, I don't mind writing comments when needed to explain a piece of code, otherwise I find it a waste of time and space.
I wonder how many developers actually read these "The something service." and find them useful, I guess we'll never know.
What I'm stil wondering about though, you said in #5 that such comments should be fixed... what should it be fixed in or did you mean something else?
Comment #11
borisson_I'm not sure about this.
I agree, there's a lot of properties where this documentation is added just to conform to the standards without adding value.
But I also agree with @drunken monkey, this would lead to more bikeshedding.
Comment #12
joachim CreditAttribution: joachim as a volunteer commented> The point of documentation is to make sure you don't have to read the code. Drupal developers are humans, not PHP parsing automatons. When scanning through a code file it is much easier and faster for a human to read "The bundle info service" than it is to read @param \Drupal\Core\Entity\EntityTypeBundleInfoInterface $entity_type_bundle_info.
Very well put!
-1 to this proposal.
Comment #13
Wim LeersThis makes perfect sense. I've long wished for the same thing. 👍
I agree with this in the specific example you cite: in the case of dependency injection.
But then you also said:
This is most definitely untrue. Nontrivial code needs comments. The examples you cite are trivial code. Not all code is trivial.
Comment #14
Dane Powell CreditAttribution: Dane Powell at Acquia commentedI think we all understand the potential benefits and downsides to enforcing comments on variables in any given situation. Sometimes it's impossible to name a variable in a simple self-explanatory way, and in those situation comments are necessary. Other times, a comment is wildly redundant and just wastes bytes and screen real estate.
The question is what's the ratio of those situations, and what should we be willing to accept as an appropriate tradeoff.
My experience is that the ratio of worthless to useful comments is quite high, and my opinion is that our tolerance for worthless comments should be low. It's one of those situations where we can enforce a standard, but we can't enforce the spirit of the standard.
The only way we can enforce the spirit of the standard is via a human reviewer, and at that point an automated rule does nothing but create noise. So +1 from me.
Comment #15
Dane Powell CreditAttribution: Dane Powell at Acquia commentedRenamed the issue to be somewhat more precise.
Comment #16
TravisCarden CreditAttribution: TravisCarden as a volunteer commentedI think there are some good arguments on both sides of this issue. Let me see if I can catalog those that have been marshaled thus far.
In favor of making comments optional:
In favor of keeping comments required:
I'd like to add a couple of arguments that haven't been mentioned against always requiring comments.
__construct()
method may not even be on the screen when you rename your class toBar
, and it's a rare patch reviewer who will think of it. I've found such artifacts in core before. This humorous code sighting may not be entirely ridiculous after all.In my mind, the strongest argument in favor of always requiring comments is probably that making them optional adds a subjective element that can't be programmatically tested for. I wonder if we could find some kind of common sense rule that doesn't have that weakness. I can imagine a satisfying compromise that begins with the following:
@var
) and function arguments (@param
) that store or take services don't require a comment because keeping language consistent across the codebase is near impossible and they provide no information that the class references doesn't. I should think it would be easy to make phpcs/Coder distinguish between primitive types and class references in order to still enforce the rule and avoid subjectivity.function testWitInvalidDates()
), 2) the audience for non-production code is much smaller, and 3) the people who read it tend to be the kind of highly skilled and experienced developers who don't benefit from being told thatEntityTypeBundleInfoInterface
refers to the bundle info service.Comment #17
pfrenssenIt looks to me that there is a pattern in the comments here, most people that are in favour of this are mentioning specifically that the parameter descriptions are pointless in the case of injecting dependencies from the container in a class constructor.
If the scope of this would be restricted to this particular case I would be in favor for this. In the case of service constructors it is clear that the parameters contain the dependencies from the container, and these are already specified in the
*.services.yml
file.I'm even fine with leaving out the entire
@param
definitions. We should require the use of the "service" qualifier in the docblock so that it is clear for users that this is a service that uses dependency injection. For example:I think that all other methods (be it public ones defined in an interface, and protected ones in individual classes) should still be properly documented.
Comment #18
slootjes CreditAttribution: slootjes at Media.Monks commentedWim Leers
I stand corrected; I think we are on the same page regarding this however you describe it in a better way.
pfrenssen
The part "Constructs a new Entity plugin manager ***service***." is exactly what I believe should not be required as it does not add value to what (code) is already there. The whole idea behind the __construct is to create a new instance of the current class, why does this need to be repeated in every class in the code base? If a function is named properly and does not contain non trivial logic I don't see the point in adding a comment.