Core: [CS] Method PHPDoc should be included even when typehints are used

[teohhanhui]

Especially since we're not including PHPDoc for class properties.

And because typehints are not always expressive enough. I think it's better that we always include the PHPDoc for all methods.

[dunglas]

I agree. Do we have methods without PHPDoc?

[teohhanhui]

Including the constructor, especially keeping in mind the single-line arguments CS which many complain about :smile:

[dunglas]

It has less sense for the constructor because we use PHP7 scalar and return types. Most modern IDEs and quality analysis tools (like Scrutinizr) are able to infer types using typehints. Duplicating this information in docblocks make the maintenance harder, the code less readable and has no interest.

[teohhanhui]

typehints are not as expressive as PHPDoc types. You can't represent a string[], for example.

[dunglas]

@teohhanhui Yes when PHPDoc is more precise (like Foo[]), it must be used. But when it has no gain (@param Foo), it should be omitted.

[teohhanhui]

I'm proposing for it to always be included, for consistency. And it's also easier for humans to read (especially those who don't use word wrap).

[theofidry]

As a rule I tend to include phpdoc whenever there is at least 1 param requiring it:

/**
 * @param Foo   $foo
 * @param Bar[] $bars
 */
function (Foo $foo, array $bars) {...}

In the case above @param Foo $foo is completely useless, but I find weird and harder to follow when you only have some parameters and IDE tend to complains when you're only giving partial phpdoc.

[teohhanhui]

It's bad when we include it sometimes, but not always. Consistency is key.

[theofidry]

I like consistency but I'm not for choosing between no phpdoc whatsoever and phpdoc everywhere. IMO adding phpdoc where necessary is good and avoid having too much noise.

[teohhanhui]

FWIW, Symfony doesn't seem to enforce anything on this...

[teohhanhui]

Sometimes the properties have PHPDoc, sometimes they don't. It's really inconsistent...

[teohhanhui]

But I think we should pick one of:

1. Always have PHPDoc for all properties.
2. Always have full PHPDoc for constructors.

[theofidry]

Always have PHPDoc for all properties.

I'm for it, as long as you can't declare the type when declaring a class property you can't tell what the property is at a glance. But @dunglas doesn't like it because he always get the auto-completion anyway with phpStorm :P

Always have full PHPDoc for constructors.

Constructor or another function: why including the doc if it doesn't bring anything? It's like:

/**
 * @return string Returns the name.
 */
public function getName(): string
{
  return $this->name;
}

It's doesn't bring any value and just pollute the class.

But that's my personal preferences anyway, let's vote for a convention and stick to it :)

[teohhanhui]

It's doesn't bring any value and just pollute the class.

It is just boilerplate. At least we could tell the documentation is not left out. And who's to say the documentation wouldn't become more elaborate? When we only add the documentation when the need arises, it's really bad for consistency... How do we decide what's trivial / non-trivial anyway? It will be one more thing to argue about ad infinitum.

I'm sure your text editor / IDE doesn't display PHPDoc in the same style, so it doesn't disturb from visually scanning the code quickly.

[theofidry]

Sorry I missed some of your comments.

And who's to say the documentation wouldn't become more elaborate?

If getName() would become non-trivial and require a comment, then yeah a doc would be needed. But when you have nothing to say about the method, I don't find useful to generate the phpdoc just for the sake of having a phpdoc bloc.

Sometimes the properties have PHPDoc, sometimes they don't. It's really inconsistent...

Until you can do private string $name;, I find useful to add /** @var string */. It's not very consistent because most of the people do it, but other like @dunglas deem it unnecessary as long as the IDE or static analysis tools are able to properly guess the type.

It is just boilerplate. At least we could tell the documentation is not left out.

Personally I prefer no documentation than an "empty" documentation (i.e. a doc that doesn't say anything.

But again we are nitpicking and that's just my personal preferences. I perfectly conceive that some people like to do things differently and I'm not enough of an extremist to be dead set on it ;). What feels more important though is picking one and sticking to it for the project.

Let's head what the rest of the team thinks about it.

[teohhanhui]

It's empty, but in a good way. No redundant summary e.g. Gets name.. No redundant description for the return e.g. Returns the name.. But the PHPDoc is there with all the types.

/**
 * @return string
 */
public function getName() : string
{
    return $this->name;
}

/**
 * @return string[]
 */
public function getNames() : array
{
    return $this->names;
}

We don't have to think / argue which one needs PHPDoc, and which does not.

[teohhanhui]

Personally I'm for PHPDoc in properties. Not all properties are from the constructor arguments.

[dunglas]

I'm too keep our CS in sync with Symfony CS. In Symfony we try too keep remove every useless PHPDoc, including docblocks of properties set in the constructor.