Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🎉 Final CS batch 🎉 #8539

Merged
merged 8 commits into from
Mar 14, 2021
Merged

🎉 Final CS batch 🎉 #8539

merged 8 commits into from
Mar 14, 2021

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Mar 13, 2021

vendor/bin/phpcs --report=summary
............................................................ 60 / 80 (75%)
....................                                         80 / 80 (100%)


Time: 3.75 secs; Memory: 82MB

I will follow up with a merge-up to 2.9.x (already tried it, it's not difficult).

This should be implemented in a separate pull request.
It cannot work when you call the private method like a callable.
We can fix it with a breaking change.
These phpdoc is parsed by doctrine/annotations, and that package does
not understand things like array<string, mixed> yet.
Now that there no longer are cs issues, we can thank diff-sniffer and
kiss it goodbye!
@greg0ire greg0ire requested a review from beberlei March 13, 2021 09:09
@@ -414,9 +414,8 @@ protected function gatherRowData(array $data, array &$id, array &$nonemptyCompon
* values according to their types. The resulting row has the same number
* of elements as before.
*
* @param array $data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes param information for non-psalm interpreters. I saw that also in older batches.
I you want I can add a PR to fix this as well as using psalm-param / psalm-var for all array<> notations.

Copy link
Member Author

@greg0ire greg0ire Mar 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure yet if psalm-param or psalm-var is the way to go or if we can use just @param. I decided to take no risk of providing information that cannot be understood and went with the prefix any time there were <>, maybe we should discuss this more in depth? What non-psalm interpreters are you thinking about? PHPStorm, probably, is there something else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't know how well IDEs like eclipse or netbeans are working with array<> for example. At least I hope there are plugins to enable understanding them.
At least we should always have @param if a typing is missing.
I prefer removing the psalm prefix for this notation. but there might be more investigation needed to find out how well it is supported in more IDEs.

Copy link
Contributor

@simonberger simonberger Mar 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some research my conclusion is that array generic notation should be added to psalm-prefixed PHPDoc attributes only.
The adaption in tools other than intellij, Psalm or PHPStan seems not existent.
It had been implemented in PHPStorm according to PSR-5

The PSR-5 chapter for collections had been removed from the draft a month later and did not find a way back yet.
Here are some more information in the PR removing the collection part
The goal was to newly start the discussion of this topic. I did not try to find out how this proceeded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this is an important discussion we need to have regularly (because tooling will evolve, and new PSRs might come up). I know we have already discussed this in the past somewhere, but cannot find where.

I think there should either be an issue for this at https://github.com/doctrine/coding-standard/issues or a contribution should be done to https://github.com/doctrine/doctrine-website/tree/master/source/policies . Can you pick one and open an issue there? That way we can point Doctrine members and people that are knowledgeable about various IDE support to it.

In any case, my point is that we should have a central location where we record this decision and what it is grounded on so that it can be found and challenged in the future, when tooling has evolved. Having a table with syntaxes and how supported they are across IDEs would help a lot.

We usually use https://github.com/orgs/doctrine/teams/doctrinecore for that kind of thing, but it is internal, so that would prevent us from discussing it with you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it makes sense that we have @param array + @psalm-param array{....}, probably a phpcs rule that can autofix the param for us, but it needs some work in the coding standard repo.

Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay!

@greg0ire greg0ire merged commit 5247c56 into doctrine:2.8.x Mar 14, 2021
@greg0ire greg0ire deleted the cs-20210311 branch March 14, 2021 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants