[Policy, no patch] Clarify point 2 of the documentation gate

I would add members of a class as a requirement for a docblock. Otherwise, this is a great clarification.

Where's the +1 button? :)

Cliff Notes version:

1. Comments must be provided that give a reasonable explanation of the codebase.
2. Comments don't have to be perfect.
3. Imperfect commenting shouldn't block a commit.

typed @param, @return, @var, and @throws wherever they are already required by 1354;

I think this should be clarified to specify whether the following is valid:

/**
 * Eats a sandwich.
 *
 * @param Ham $ham
 * @param Cheese $cheese
 *
 * @return Joy
 */
function eatSandwich($ham, $chesse) {...}

or whether for that to become valid, it would have to be:

/**
 * Eats a sandwich.
 *
 * @param Ham $ham
 *   The ham to put on the sandwich.
 * @param Cheese $cheese
 *   The cheese to put on the sandwich.
 *
 * @return Joy
 *   The joy you have now that you've eaten a sandwich.
 */
function eatSandwich($ham, $chesse) {...}

I think for a strict gate the former should suffice, but I don't feel strongly either way.

Unless I misunderstand our documentation standards, I believe an explanation is needed for each @param and @return directive. Hence, the second part of #4 is the expected documentation standard (at least as I understand it).

The second example in comment 4 is what I would require. The first tells me nothing I cannot get from the function signature itself. Although technically we'd want to be type hinting in the signature, too.

Otherwise, +1 and agree with Lars in #1.

Echo Crell in #6.

But I'm not too fond of the current wording of the minimum requirements in the issue summary, because I don't think it makes the requirement clear. The current proposal is:

A one-line summary for all interfaces, classes, methods, class members, functions, and constants; typed @param, @return, @var, and @throws wherever they are already required by 1354; inline comments to explain the code flow; and explanations for anything complicated.

I propose this instead:

A documentation block for all files, interfaces, classes, methods, class members, functions, hooks, and constants, and inline comments to explain the code flow. The documentation block must contain a one-line summary; typed @param, @return, @var, and @throws (with descriptions on param/return) where required by http://drupal.org/node/1354; and longer explanations for anything complicated.

The proposal adjusted wording in #7 looks good to me!

Yes

I think we should also point out, what is explicitly not part of this gate, as per #1791872: [Policy, no patch] Add special tag to identify issues ready for a high level only review. E.g. typos, missing leading backward slashes for namespaced classes, minor grammatical issues (hyphen/no-hypen, AE/BE spelling, ...), blank line missing after class declaration, and so forth.

Of course we should not encourage that, but reading #7 I wouldn't really know what is allowed for the Docs gate, that is not in the coding standards anyway.

I suppose we don't have to list every little detail that you're allowed to do, but a general sentence "Typos and other minor documentation errors, although obviously not encouraged, may be fixed in a follow-up." would help IMO.

Good point. Revised (possibly controversial) proposal:

A documentation block for all files, interfaces, classes, methods, class members, functions, hooks, and constants, and inline comments to explain the code flow. The documentation block must contain a one-line summary; typed @param, @return, @var, and @throws (with descriptions on param/return) where required by http://drupal.org/node/1354; and longer explanations for anything complicated. The documentation must be clear and complete, must fully explain what's going on, and must contain data types; typos, verb tenses, formatting, and the like may be cleaned up in follow-up issues for large patches where it would hold up progress, at the committer's discretion.

I like the checklist format. Looks good to me @xjm.

+1 on #12 (either format, whichever is better for the gates page).

Looks good to me, thanks! Tentatively marking "fixed" since we all agreed upon the update, and both xjm and I think it's been added properly to the Gates page.

If the community is still willing to listen to me (which I doubt): don't do this.

I called this practice out in https://medium.com/@chx/the-terrible-cycle-of-contributing-to-drupal-cor... but perhaps it was not clear enough: we demand doxygen for every property, parameter and method when it absolutely makes no sense and it's automatic and robotic to fill in and we never demand meaningful documentation on the other hand. It's a farce.

What does this

  /**
   * An array of build info.
   *
   * @var array
   */
  public $build_info = array();

achieve aside from making the file longer and thus harder to read? There are a million of these "we must have doxygen for everything". Yes, it satisfies phpcs, you get a cookie, who cares. We got to the point both with docs and tests where we do them mindlessly and their value is now often negative.

On the other hand when real documentation is needed we do not have it and we do not enforce it. At all. For example, go ahead and tell me based on the doxygen what should you expect from any typed data getValue call and what format should you pass into any setValue call. It's not there. It's just not there. Map::setValue() has this to say "An array of property values". What are the keys, what are the values? Anyone's guess. And most of the time it's just "@inheritdoc", even when it shouldn't be.

Two years after I filed an issue for it, ContextInterface still proudly announces "Interface for context.", so don't tell me I should go ahead and file issues. I tried (that's two links and probably there are more but these are the two that I remember most).

This policy cements the requirement for negative value doxygen and divert attention from the incredibly pressing need of real doxygen.

We need to change https://www.drupal.org/core-gates#documentation-block-requirements and make the one line summary optional on the other hand enforce with an iron hand the "Longer explanations for anything complicated"! When was the last time an API addition or change was refused because of missing meaningful doxygen? I am not talking of setting to CNW because of drive-by phpcs sniping I am talking of a thoughtful review which refused until good doxygen was written?

Make it a target for 8.2 to fix the missing doxygen from Drupal 8. If you need a list, I can give you one. I know it's endless but we can find a list of major API calls which are completely missing documentation. I already gave you a start here: the two issues linked and the Typed Data structure in getValue / setValue.

While I don't feel we should stop documenting simple things, I otherwise entirely agree with chx; too often we stop at basic information in docs for formality's sake and don't provide the documentation actually needed. The perfunctory documentation is fine, but we do need to go further.

Here's a good guideline to start with: If returning a complex data structure (ie, anything more than a primitive or a sequential array of primitives), the structure of that data MUST be explicitly defined. That could be returning an object of a typed class (the class definition therefore defining the data structure, which is what it's there for) or a detailed example/description of the nested array structure. The former is, of course, preferred, as the PHP type system then provides automatic validation and documentation for us, making the whole job easier.

> While I don't feel we should stop documenting simple things,

does not answer

>> what does this achieve aside from making the file longer and thus harder to read?

> While I don't feel we should stop documenting simple things,

does not answer

>> what does this achieve aside from making the file longer and thus harder to read?

IMHO this is not at all the actual problem we deal with. Whether adding short bits, or not, leads to better documentation in general might be a valid discussion to have, but the actual underlying problem is much deeper: The lack of value of good documentation is more of a social problem, rather than a technical/(missing, redundant) rules problem. Much like you cannot solve many problems of bad behaviour online by introducing custom technical solutions, you cannot improve the documentation by adding or removing some small rules.

It seems to be that over the years, since, I'm not sure actually, we managed to value testing quite a bit. We started with functional tests to ensure that we can sleep better at night, and even managed partially to understand that testing is not necessarily just about that, but also about the interesting correlation between good testable code and high quality code. We managed to introduce the social change that testing is a high value, and by that we often produce good testing without even thinking about it.

I think we want to have a similar culutural/social change in the way how we deal with documentation as well. Of course this is super hard to achieve, but I think some of the following points help:

• New contributors which don't know the details help us to realize, where things aren't explained properly yet. They have a similar perspective than the everyday person.
• Having consistent rules, like maybe the rule @chx and @crell argue about, help people to not worry about the inconsistency of the rules, but rather focus on improving the actual docs
• Providing good reviews on proper documentation helps people to understand the high value of documentation, which leads to better views in other places ... As laid out, social changes requires time.
• Let's have an example how we could provide valueable information for the various properties on a service actually:
  

  class State implements StateInterface {
  
    /**
     * The key value store to use.
     *
     * @var \Drupal\Core\KeyValueStore\KeyValueStoreInterface
     */
    protected $keyValueStore;
  

  and we could improve that to something like this:

  class State implements StateInterface {
  
    /**
     * The key value store which is used as underlying data storage of state.
     *
     * The default implementation of state uses a key value store to store its data,
     * but it could be another data storage, because state have a custom semantic meaning
     * on its own.
     *
     * @var \Drupal\Core\KeyValueStore\KeyValueStoreInterface
     */
    protected $keyValueStore;
  

PS:

What does this

Let's be constructive and not destructive: #2768115: Improve documentation of ViewExecuteable properties

> It seems to be that over the years, since, I'm not sure actually, we managed to value testing quite a bit.

I am not sure how much was it value and how much was it enforcement. Even I can't remember when did we start refusing patches without tests but we certainly do. (I am guessing 2011 https://groups.drupal.org/node/158774 )

Edit: re constructive vs descructive, that not my intention, I could quote the 43 services which gets the module handler injected and meticulously documented on the constructor that ModuleHandlerInterface $module_handler to everyone big surprise is actually the module handler and then the moduleHandler property gets the same treatment because noone ever would think that might be, you know, perhaps, the module handler! Same with 19 services getting @database injected and so on and so on, there's hundreds of them altogether.

I am triaging issues tagged 'coding standards'.

This issue was marked fixed 9 years ago and then re-opened 3 1/2 years later to ask that the change not be made. After the re-opening there were 4 more comments from 3 people, the last being a few days later. There has been no further discussion supporting a change to the current core gates documentation requirements. Therefore, restoring the 'fixed status' and version.

Anyone wanting to propose changes to the Drupal coding standards can do so by making an issue in the Coding Standards project.