Add psalm-prefixed annotations to param and return group

[simonberger]

No description provided.

[ostrolucky]

While we are it, we should also add phpstan- variants. Also, tests are usually needed

[greg0ire]

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

[simonberger]

I added tests for all new annotations.

[greg0ire]

I think more tests are needed to take a decision

[greg0ire]

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.

[simonberger]

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.

[greg0ire]

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:

https://github.com/squizlabs/PHP_CodeSniffer/blob/2bd19c2e6edb918ab14d87f1c4b37d1f112d8f00/src/Standards/Squiz/Sniffs/Commenting/FunctionCommentSniff.php#L296

Let's drop that second remark.

[simonberger]

@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.

[greg0ire]

@simonberger I was unsure and wanted to see what happened, as well as other reviewers. IMO it's already fine like this.

[greg0ire]

Please associate your email address with your Github account, or change the
email in your commits to an address already associated with it. If you do not
want to expose your personal email address, you may use
$userName@users.noreply.github.com, that way we can still reach you through Github.
Additionally, you will get credit for this contribution on your Github profile.

[greg0ire]

To be squash-merged

[simonberger]

Yes I'll fix this. Forgot to override the git email for the fork.

[simonberger]

Rebased and changed email.

[morozov]

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> $foo

[simonberger]

Hi @morozov we discussed this here #238 (comment)
It is not possible from the phpcs package.

[simonberger]

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.

[ostrolucky]

Yes it's not the goal of this PR, would be a scope creep

[greg0ire]

Thanks @simonberger !

[greg0ire]

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 ?

[simonberger]

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.

[greg0ire]

I'll ask for guidance internally.

[greg0ire]

To quote @alcaeus

The stance was “if it introduces new errors that aren’t due to bug fixes, it’s a bc break”. Exceptions apply where even a bug fix may only be applied in the next major depending on impact (e.g. thousands of errors in our own projects)

I will revert this from 8.2.x and cherry-pick it on 9.0.x

[beberlei]

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.