wizacode/php-etl

Type hinting and PHPDoc

Closed this issue · 2 comments

In another issue, a question was raised about a function's PHPdoc - in particular, the function had been like this

/**
 * @param string $a
 * @param array $b
 */
public function foo (string $a, $b) {...}

I removed "@param string $a" in order to pass the inspection "no_superfluous_phpdoc_tags" (which is implied by @symfony in the distributed .php_cs.dist file). That raised a question among the reviewers and the statement that one reviewer would like to see PHPdoc type hints for all parameters.

I'm creating this issue as a locus for any follow-up conversation.

If we want to see PHPdoc type hints for all parameters, we would have to follow one of these courses:

  • remove php-native type hints (which would reduce code reliability)
  • provide comments on each variable (which tends to generate a lot of repetitive and uninformative text)
  • override the configuration and set "no_superfluous_phpdoc_tags" to false

I can live with any choice that makes the code and tests comply with the inspection profiles we ship alongside. But I prefer the configuration we have. In my mind, the desired ideal is that nearly all variables are self-documenting and are constrained by php-native types. With the current codebase, we can verify that things like

ETL::load(Loader $loader, string $output, $options = []): Etl {}

can be refactored to

ETL::load(Loader $loader, string $output, array $options = []): Etl

It seems implied by the code, but since that could introduce a breaking change, so it also seems worth explicitly verifying.

There are about as dozen places in the code where the variable type is mixed. With the current inspection profiles, those variables would need to retain their PHPdoc. In all other cases, PHPdoc would be provided for variables only when the variable name was not felt to be adequately self-documenting.

There hasn't been any comment in this, but bits and pieces in other cards lead me to believe we want to avoid writing redundant PHPdoc when we have type hints for properties with PHP 7.4 and when we make that release we'll be removing stuff that becomes redundant.

Does anyone disagree?

I personally have a preference for adding PHPDoc tags for all parameters. I have no authority regarding this package, so my opinion might not be worth much :) But I prefer this way because, even through it seems redundant, it's easier to scan for missing/incomplete PHPDocs if the number of tags matches the number of parameters. I believe it helps reduce the number of mistakes.