CS: apply phpdoc_annotation_without_dot reworked #20366
Conversation
|
Is there a reason to capitialize some descriptions, but to lowercase others? It's seems quite inconsistent to me. |
|
The intention for original rule was that the annotation description should not be a sentence, so we trim trailing dot. And if it's not a sentence, the leading char shouldn't be capitalized. |
|
Hmm, alright. Still it's weird that one-line descriptions like https://github.com/symfony/symfony/pull/20366/files#diff-70d56b3a86265721eccd933af82b7332R113 aren't fixed. Maybe the fixer only checks for final stops and not capitialized words? |
| @@ -108,15 +108,15 @@ | |||
| * | |||
| * @param Definition|Reference $driver Driver DI definition or reference | |||
| * @param string[] $namespaces List of namespaces handled by $driver | |||
| * @param string[] $managerParameters List of container parameters that could | |||
| * hold the manager name. | |||
| * @param string[] $managerParameters list of container parameters that could | |||
There was a problem hiding this comment.
I am okay with removing the dot, but changing the case here makes the docblocks very inconsistent as most places start with a capital letter.
There was a problem hiding this comment.
Actually it's the same comment like yours @wouterj .
Just to remind - idea behind fixer was that the description should not be a sentence, so it shouldn't have trailing full stop. In that case - it shouldn't start with capital letter as well.
From what I can remember it's the left-over from first implementation, where the dot was removed but capital letter was not modified.
Remembering the original goal - what about changing capital letter as well, even if trailing dot was already removed before ? If one would run only 2nd implementation of fixer (without 1st) - that would be the outcome.
There was a problem hiding this comment.
I fear that we will then have many changes that will cause lots of conflicts when merging branches.
There was a problem hiding this comment.
Indeed, we won't merge PRs that makes merging difficult.
|
👎 for me, unproven positive value, clear negative one (merge conflicts) |
|
ref #19198 |
|
Closing as won't fix: as commented, that'll creat merge conflicts - a final dot on comments doesn't deserve it (especially since we have so many PRs pending ;) ) |
|
@nicolas-grekas Could you please specify is it a rule that would be nice to apply in new code, but not old (for the conflicts reason), or no willingness to follow it in new code as well ? Especially, as the rule was initially requested by Symfony. btw, merging between own branches (like 2.x->2.y->3.z) could be make like:
(I know, it won't fix potential conflicts with branches on forks, where people work on PRs) |
|
@keradus not sure I can tell you really... Right now, we have E_TOO_MANY_PR, so I've been a bit strict here, to help clean the backlog. The fix might happen later I guess (but would be great to have fabbot not patch the case of the first letter first :) ) |
|
So, some CS rules that Symfony wants, but not apply them, as of conflicts with merging. Cool to loosely follow it on new PR, but not in old, existing code. |
phpdoc_annotation_without_dotrule was requested by Symfony, most cases was adjusted in code, but the rule was a bit problematic to apply it everywhere.This PR is based on extended implementation of that rule (see PHP-CS-Fixer/PHP-CS-Fixer#2247). I think it covers the problematic cases properly, but let us discuss.
Also, for that reason, I decided to put it in independent PR.