Php-cs-fixer: Fixers should be forced to have a rationale

[PowerKiKi]

I would like to start a discussion about the need for every single fixers to be documented with their rationale. This should then appear in the describe command, and in the documentation (README.rst).

For now all fixers must have a summary and may have a description. The description usually explains in details _what_ it does, but it rarely says _why_ we should enable that particular fixer. This makes it difficult and/or time consuming to decide whether or not new fixers should be enabled during an upgrade.

A recent example of this is the StaticLambdaFixer. From the describe command it is extremely clear _what_ it does, but there not even a hint of _why_ it may be a good idea to consider it:

$ ./vendor/bin/php-cs-fixer describe static_lambda
Description of static_lambda rule.
Lambdas not (indirect) referencing `$this` must be declared `static`.

Fixer applying this rule is risky.
Risky when using "->bindTo" on lambdas without referencing to `$this`.

Fixing examples:
 * Example #1.
   ---------- begin diff ----------
   --- Original
   +++ New
   @@ -1,4 +1,4 @@
    <?php
   -$a = function () use ($b)
   +$a = static function () use ($b)
    {   echo $b;
    };

   ----------- end diff -----------

This is even exacerbated by @localheinz who kind of asked about it in the PR, but never gives an answer.

A counter-example of that is ExplicitStringVariableFixer whose description contains a rationale, and makes it much easier to make a decision about it:

$ ./vendor/bin/php-cs-fixer describe explicit_string_variable
Description of explicit_string_variable rule.
Converts implicit variables into explicit ones in double-quoted strings or heredoc syntax.
The reasoning behind this rule is the following:
- When there are two valid ways of doing the same thing, using both is confusing, there should be a coding standard to follow
- PHP manual marks `"$var"` syntax as implicit and `"${var}"` syntax as explicit: explicit code should always be preferred
- Explicit syntax allows word concatenation inside strings, e.g. `"${var}IsAVar"`, implicit doesn't
- Explicit syntax is easier to detect for IDE/editors and therefore has colors/hightlight with higher contrast, which is easier to read

Fixing examples:
 * Example #1.
   ---------- begin diff ----------
   --- Original
   +++ New
   @@ -1,4 +1,4 @@
    <?php
   -$a = "My name is $name !";
   -$b = "I live in $state->country !";
   -$c = "I have $farm[0] chickens !";
   +$a = "My name is ${name} !";
   +$b = "I live in {$state->country} !";
   +$c = "I have {$farm[0]} chickens !";

   ----------- end diff -----------

Other projects, such as TSLint are doing that very well and have a short and clear rationale for every single rule (see example).

So I would like to suggest the following steps:

1. As of right now, reject all new fixers who don't include a rationale in their description
2. Enforce that requirement by code
   
   
   
   1. Come up with a new signature for FixerDefinition that includes a mandatory rationale, separate from description
   
   
   2. Update all fixers accordingly
   
   
   3. Show that new info in documentation
   
   
3. As a bonus, generate one page per fixer to document them, which would include pretty much the output of describe command, because the single list in the README is getting very long

[SpacePossum]

I'm all for better docs, but I've a few points here.
Point 3 is not a bonus, it is a lot of work no one finished so far.
Point 2.ii seems fair, but typically isn't, why? Because when the FixerDefinition was introduced and had to be completed for each fixer, it was up to a few people to do so. I think the same will happen here, the original authors of the fixers are not just coming back and write the rationale.

I would not consider any of the points, especially point 1., until at least, lets say; for half of all the fixers on master plus one, the rationale have been written and agreed upon by the community in a PR.
That way we know 3 things:

1. there is enough interest
2. it is doable
3. how the new docs look like

[julienfalque]

I started working on documentation improvements a few months ago, I submitted #3475 so we can discuss it.

[keradus]

FixerDefinition has a place to put rationale already, you saw it in explicit_string_variable.
So, we do have almost 200 rules so far. Plus a lot of them are configurable, and config could create a reverse rule, which needs rationale as well.
So, @PowerKiKi, I'm super happy to enforce that all fixers has to have it. Really big :+1: on that.
As soon as I see a PR for 2.2 providing that rationale for currently existing rules.

[PowerKiKi]

I cannot tell if @keradus reply is being sarcastic or not, so I am not sure how to reply to that. But just in case if it isn't, wouldn't you say that a description is not the same as a rationale and thus it should be two different fields ? That way the description would be kept optional could be as short or as long as needed, while the rationale would be mandatory and be as concise as possible ?

@SpacePossum by not considering point 1, you seem to be saying that you refuse to increase the quality of newer code on the pretext that existing code is of lower quality. That does not sound very motivating for the people trying to "fix it" if somebody else keep adding broken stuff. Would you reconsider your position on that ?

Also I forgot to mention in my original post that we could also consider categorizing fixers. Some of them are purely aesthetic, such as no_extra_blank_lines. While others might be performance related, such as native_function_invocation. And others might be about strictness (strict_param). Would there be any interest/use for something like that ?

[SpacePossum]

I stated I would not consider any of the points, especially point 1., until , so clearly I would consider it as the until part is matched.

So to That does not sound very motivating for the people trying to "fix it" if somebody else keep adding broken stuff.
I'm stating: please people fix it! Very much welcome, send it in as a PR and I merge it in heart beat. This can be done as @keradus stated within the current definition. We can split it out later when needed.
This is not sarcastic, we will merge any documentation improvements we get in PR form.

Till then, however, I'm not going to stop merging good fixers, like you suggest in point 1. You consider a good working fixer still broken stuff if the docs are not on some level, I think it is not broken and very demotivating for people to see their work not merged over wording in the docs.

All in all we all like better docs, so lets get these first _before_ we start making changes to the definitions, interfaces and stop merging other work. Maybe a good start would to look at the current PR's with proposed new fixers and see if these have an acceptable rationale in the description?

[PowerKiKi]

Thank you for the clarification on your position. I most likely won't invest more time in this, so I'll be closing the issue for now. If other people join the discussion in the future, and have I have more time at that point, I might join back in though.

[keradus]

@PowerKiKi, I'm super serious on that !
As @SpacePossum mentioned, we were doing exactly the same thing once already - with summary of rules.
We didn't enforced it until we provided them for all rules. It was pretty big thing to do.
I would be super happy to see rationate for the rules, and your help with providing them is much appreciated !
Please, start working on it ;)

In definition of rule, we do have 2 fields, summary and description. Summary is super short (one-liner) info about the rule, it's mandatory, while description was planned especially to contain the rationate. No need for new field.

My 2 cents for your further discussion with @SpacePossum.
Sadly, it's super common that one who create a great code, can't create at least decent docs for it.
So for long we have the field optional, let us not block new rules by lack of it.

---

Summarizing the thread. It's great idea to have rationale for all rules, but it's so big thing to do, that even initiator of idea don't want to make that happen.