Are typed @param / @return part of the documentation gate?

AFAIK the noncommittal language was the result of these things:

• The datatypes actually weren't anywhere in core, and we were reluctant to introduce a new requirement in that circumstance.
• Several people were concerned at datatypes being added piecemeal or inconsistently.
• Drupal 7 was still the branch of Drupal with most of the focus, and we agreed not to backport the standards change to a production version.

That was over a year ago, though. IMO a datatype on your @param or @return is a part of the standard for D8. "Obvious" often ain't. So I'd remove the ambiguous language (edit: and add typehints throughout 1354 and other handbook pages).

In proper spec speech, both items should say "SHOULD specify"

There's some desire to turn the item about @return into a "MUST specify"

The "obvious" part refers to small helper functions like the following, which do not have @return directives currently:

/**
 * Returns the ID of the passed in thingie for the current language.
 */
function thingie_get_id(Thingie $thingie) {
  $language = language(LANGUAGE_INTERFACE);
  return $thingie->getID($language);
}

Agreed with sun in #3. And yes, I'm one of the people favoring a MUST on @return. It may be obvious to us looking at that code sample, but an IDE will never auto-complete that. If you're returning an object and don't specify a parseable return type IDEs won't be able to auto-complete, code hint, or do any of their other neat IDE-ish things.

I'd be fine with it becoming a MUST in both cases for consistency.

(And also with us adopting proper RFC definitions of MUST, SHOULD, etc. :-) )

Um... What exactly are you proposing be added to or changed in the current Documentation Gate or Documentation Standards pages? (which sections, before/after would help)

I wasn't really in favor of making types in param/return part of the docs standards, because most of core didn't comply (obviously) and the cleanup patches are difficult to write and review (you have to actually read the whole function to figure out the types in many cases). But everyone seemed to want that standard, so we adopted it... So I'm not the right person to ask whether we should make it part of the "gate". I will hereby abstain from adding an opinion to this issue, except to say that if we make that standard into "you are encouraged", I will not complain. :)

And yes we probably should fix node/1354 so that every param/return has a data type on it, so that at least our own standards page follows our own standards, if it doesn't currently (if that was the suggestion in #6).

I am very much a fan/proponent of type hinting for @param and @return directives. It helps IDE editors as well as the documentation that shows up on the API website. It also really helps to use a function without having to inspect it as @jhodgdon points out in #7!!

The best persons to make the type hint suggestions are those who are developing or modifying it; not someone coming along later trying to understand what the developer meant. As a result, I am a very big +1 on this being part of our coding/documentation gates.

Also, committing such @param and @return type hinting clean-up patches should not solely be left to @jhodgdon! To get core cleaned up, I think that we are going to have to create smaller patches like all functions in a single module or file. For bigger beasts like common.inc, we might want to limit such patches to only touching X number of functions (10 - 25??). Feedback from core committers are welcome on what number of functions to target so the patches are not too hard to review and commit.

Type hinting really will help the Drupal community!

I'm +1 on MUST, too.

However, as long as core does not follow the standard yet, I'd prefer to keep it as a SHOULD.

Also, we still have to figure out what to do about trivial helper functions for which the phpDoc summary sufficiently explains what is being returned already. I'd hate to see duplicated docs.

It would probably make most sense to add an exception to the rule for @return, allowing it to exist without description in those cases; e.g., repeating the example from #3:

/**
 * Returns the ID of the passed in thingie for the current language.
 *
 * @return integer
 */
function thingie_get_id(Thingie $thingie) {
  $language = language(LANGUAGE_INTERFACE);
  return $thingie->getID($language);
}

RE #9 - our standard in the past for these types of functions has been to leave out the @param/@return entirely. IMO, if we have @param/@return, they should be fully-realized documentation, not a stub like this. I'm totally against adopting #9 as a standard way of doing things.

@sun Thanks for the updated example in #9. Generally, if I am going to use the helper function, I do not care at all about how Thingie is implemented. I only care about what do I need to supply and what do I get back.

Without @return integer, one would have to open up Thingie looking for the getID method to see what it is returning. Yes, in most cases, it probably is a serial integer, but in your example, it could well be a string like 'es' or even a uuid. Of course, if Thingie class uses an interface, which is Drupal's best practice, the documentation of what getID() returns is in that second file. Documenting what type of ID is being returned is most helpful on the helper function, even if it is duplication of other documentation.

Hence, I am a strong proponent that all @return and @param diectives MUST have type hinting and appropriate description. I also feel that patches should not be committed without this.

Finally, I believe, if I submit a patch changing something in function x(), it is my responsibility as the generator of that patch to check and update the function x() docblock appropriately as if this were a new function. Reviewers and committers then would see incremental docblock improvements appearing as part of our normal review and commit process.

Following up on @sun's comment in #9 that "as long as core does not follow the standard yet, I'd prefer to keep it as a SHOULD", I have opened up #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing* to move existing core code to compliance with this issue.

Please remember that "type hinting only" patches are time consumming to review and commit, and hence tend to have a lower priority when there are competing issues that need attention. Hence, the best way to get type hinting added is add correct @param and @return directives when one is developing or modifying particular functions.

I keenly appreciate that "after the fact" type hinting patches are difficult to review and commit. However, how are we suppose to make core compliant if a core committer responds:

I do not have Drupal volunteer time that I want to spend any time soon reading the code carefully just to commit a clean-up patch (there are several other clean-up patches waiting in the queue, and API module improvements to do, etc.). Sorry, that's just how it is.... Type hinting is a separate effort, and it is that way for a reason.

I am at a loss as to how to proceed.

Since I am that "core committer" mentioned, let me clarify.

I have been given permission to review and commit "obvious" patches, without an extra reviewer. Meaning that I'm willing to commit submitted patches that haven't been reviewed by another contributor -- but only for some patches (grammar fixes, documentation formatting, etc.). Like code patches, which pretty much *always* require a reviewer to mark them RTBC before the committer comes along and gives them a final review before committing, type hinting patches require that the reviewer carefully read the code and see if the types put in the param/return statements are correct. That requires a lot of effort, and is prone to error, so I'm not willing to just review/commit in one fell swoop on type hinting patches.

That comment (which was taken out of the context it was added to -- a doc cleanup effort that *specifically excluded* type hinting as part of that effort) was not mean to imply that I wouldn't *commit* type hinting patches (on appropriate issues) if they've been reviewed carefully by other reviewers, just that I wouldn't do a one-swipe review/commit on those patches.

So let me ask "Are typed @param / @return part of the documentation gate?" And if the answer is yes, how do we move existing core code toward compliance?

Fair enough @webchick. I have never expected a patch to be committed without a review. However, if a patch includes type hinting, I would expect that such a patch, if it has been reviewed, would be considered in due course for a commit. Are my assumptions wrong?

Your assumption is correct in #17 (except for massive clean-up initiatives in which type hinting has specifically been excluded to make the patches easier to review/commit, of which I am aware of two going on currently).

The more I work with D8's ever-increasing OO codebase, the more I feel a datatype on the @param, @return, @var, and @throws is essential. It's really difficult to understand how unfamiliar classes and interfaces interrelate without them, but far more grokable with a proper datatype and typehint. Every single time I look at a new class, the "ah-ha" moment when I see how everything fits together is when I see a datatype that tells me where another class goes and how it's used. When you have a typehint for a class or interface, you know what it contains and what methods you can use on it. If you don't, have fun playing hide-and-seek in the codebase to figure it out.

Plus, as has been stated repeatedly before, it's really really hard to add the datatypes after the fact, but the author of the code presumably knows exactly what they are and what they're for.

Note that I'm talking about changes to functional code. I'm not talking about pure documentation cleanups, which I've come to believe are more successful when the scope is narrow and well-defined. Scope creep is killer in docs cleanups, and it drains my energy as a reviewer. IMO whether datatypes should be included in documentation patches can be at @jhodgdon's discretion, on a patch-by-patch basis even.

Edited to remove nonsensical typos.

Filed: #1809354: [Policy, no patch] Clarify point 2 of the documentation gate

I guess this was already taken care of elsewehre.