Add psalm-prefixed annotations to param and return group #238
Conversation
There was a problem hiding this comment.
I had no idea it would be that easy 👍
Please add some tests though, so that we are sure what the result is… I supposed there will be no constraint on the order of elements inside a group but let's check. Also, it would be interesting to see what it means for alignment of types, variable names and descriptions
|
I added tests for all new annotations. |
tests/fixed/doc-comment-spacing.php
Outdated
| * @param int[] $foo | ||
| * @param int[] $bar | ||
| * @psalm-param array<int, string> $bar | ||
| * @phpstan-param array<int, string> $bar |
There was a problem hiding this comment.
Since there are only 2 parameters here, it's impossible to tell if the @psalm- and @phpstan- annotations are last because $bar is the last parameter, or because they are @psalm- and @phpstan- annotations. Also this snippet shows that types are aligned, but variable names are not, and does not show what happens to descriptions.
There was a problem hiding this comment.
Entries are sorted also inside the group by description of the tool. I don't think we have to test this but I could add it. Variables aren't aligned in the group at least not beyond same annotation name. I don't know what to do about it. The phpcs + extension used here is not good documented and might have conflicting settings.
Maybe someone with more insights of the tool wanna have a look at it if this is a requirement.
There was a problem hiding this comment.
Can you annotate $foo instead of $bar? That will take care of my first remark. Also, int[] is not compatible with array<int, string>, it would be less confusing to have array<int, int> or array<string, int>` here.
Regarding the second remark, I don't think it should be a requirement to have the variables and descriptions aligned, and I thought it shouldn't come as a surprise to reviewers that would approve this, but maybe what you are saying is that it would be wrong to "enforce" not aligning with a test.
Also, it looks pretty hardcoded, so there is not much hope to get it fixed quickly:
Let's drop that second remark.
There was a problem hiding this comment.
@greg0ire I did the change in the test. Did you expect a correction of the param order? This did not happen and seems not to be part of the tool (or it had to be configured.
There was a problem hiding this comment.
@simonberger I was unsure and wanted to see what happened, as well as other reviewers. IMO it's already fine like this.
|
Please associate your email address with your Github account, or change the |
|
Yes I'll fix this. Forgot to override the git email for the fork. |
|
Rebased and changed email. |
| * @param int[] $foo | ||
| * @param int[] $bar | ||
| * @param int[] $foo | ||
| * @param int[] $bar |
There was a problem hiding this comment.
Is it intended that parameter types are aligned regardless of the vendor prefix but the names aren't? Since we assume prefixed and non-prefixed to have the same meaning, I'd expect them to be aligned in the same way:
* @param int[] $foo
* @param int[] $bar
* @psalm-param array<string, int> $foo
* @phpstan-param array<string, int> $fooThere was a problem hiding this comment.
Hi @morozov we discussed this here #238 (comment)
It is not possible from the phpcs package.
There was a problem hiding this comment.
If you are interested in having this formatting I am pretty sure adding another configuration option to phpcs shouldn't be too hard. But from the structure of the code I don't want to spend time getting into it to contribute there.
There was a problem hiding this comment.
Yes it's not the goal of this PR, would be a scope creep
|
Thanks @simonberger ! |
|
Should this have targeted 9.0.x instead? 🤔 I'm not sure of what is considered a BC-break in this repository exactly, but this feels like it will have a big impact on the code base of projects using this standard won't it ? |
|
I cannot really say much to this or even give an advice. Almost any coding standard change could be called a breaking change. It should just affect changes on new code (in most projects), but then can report issues on code the recent PR did not introduce. |
|
I'll ask for guidance internally. |
|
To quote @alcaeus
I will revert this from 8.2.x and cherry-pick it on 9.0.x |
|
I would assume every change to the standard should get its own major, essentially leading to always just 9.0.0, 10.0.0 etc since its seldom that we have real bugs. |
No description provided.