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.

Sign up for GitHub

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

Jump to bottom

Adding details to types in PHPDoc #8548

Merged
merged 1 commit into from
Apr 18, 2021
Merged

Adding details to types in PHPDoc #8548

merged 1 commit into from
Apr 18, 2021

Conversation

orklah
Copy link
Contributor

@orklah orklah commented Mar 16, 2021

This PR propose detailing phpdoc for the most part. Most types were generated directly by Psalm and refined by hand.

greg0ire
greg0ire reviewed Mar 17, 2021
View reviewed changes
lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php
@@ -399,7 +399,7 @@ protected function getDeleteSQL(PersistentCollection $collection)
/**
* Internal note: Order of the parameters must be the same as the order of the columns in getDeleteSql.
*
* @return mixed[]
* @return list<mixed>
Copy link
Member

Choose a reason for hiding this comment

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

Should this be duplicated because of the list part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

list is now an alias of array in phpstorm. Not sure in VS Code

greg0ire
greg0ire reviewed Mar 17, 2021
View reviewed changes
lib/Doctrine/ORM/Query/ResultSetMapping.php
@@ -428,7 +430,7 @@ public function isScalarResult($columnName)
*
* @param string $alias
*
* @return string
* @return class-string
Copy link
Member

Choose a reason for hiding this comment

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

This is not understood by IDEs is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's now an alias of string in PHPStorm. Not sure in VS Code

@greg0ire
Copy link
Member

How closer to Psalm and PHPStan's next levels does this bring us?

@greg0ire
Copy link
Member

@vladyslavstartsev @simonberger please review 🙏

@orklah
Copy link
Contributor Author

orklah commented Mar 17, 2021 •
edited
Loading

How closer to Psalm and PHPStan's next levels does this bring us?

It wasn't quite my metric, but with issues up to level 3, it fixes 63 issues, it revealed 13 new ones and upped the type coverage from 90.4980% to 90.5595%

@orklah orklah force-pushed the test2 branch 4 times, most recently from ec30919 to 5f27ad9 Compare April 17, 2021 06:51
@greg0ire
Copy link
Member

Why are there 2 commits? What is the difference between "psalm fixes" and just "fixes"?

@orklah
Copy link
Contributor Author

orklah commented Apr 17, 2021

!'m still working on that, I'll squash them later

@orklah
Copy link
Contributor Author

orklah commented Apr 17, 2021 •
edited
Loading

This should be good now :)

This has been rebased and I fixed the new issues related to the baselines that were introduced.

I had to change a few things:

  • I updated phpstan version. The version that was used before didn't support PHP8 so I couldn't work with it in my dev environment
  • I introduced phpstan/phpstan-webmozart-assert. The library is used at some point and phpstan needs the plugin to be able to understand what it does (EDIT: not anymore, I must have added the only usage myself and removed it since)
  • I added a set php version to phpstan config. This will ensure everybody runs phpstan against the same version (namely the oldest version supported by the branch). This is needed to have a coherent baseline generated no matter what php version used to run phpstan itself (I was getting issues with different errors in PHP8 than in PHP7.4 on CI and even in PHP 7.2). Note that this is not the recommandation for this: Issues should have the same error message between PHP versions phpstan/phpstan#4871 (comment) but going further is not in the scope of this PR at all. It may be adressed later.

@greg0ire
Copy link
Member

greg0ire commented Apr 17, 2021 •
edited
Loading

Your last message has bullet points containing interesting information. Could you please make one commit per bullet point, and reuse what you wrote to create a commit messages? Some of these commits should probably target 2.8.x. For instance, to avoid issues when merging up, it would be great to have the same versions of phpstan on all branches.

The second one can stay on the 2.9.x PR I think, because it will have no effect on the CI of the end user.

The third one should be contributed to 2.8.x, since it does not affect the code itself but rather the build process, and as you pointed out, it seems important to have the same messages, especially when merging up (we don't want to have complicated things to do when merging up). Since it is a temporary solution, it should probably come with a comment saying we should remove it when we successfully remove the baseline?

And finally, some changes in this PR are type improvements, while some others are type fixes (the types were just plain wrong before, for instance when we use int instead of int|false as a return type. Using git reset origin/2.9.x && git add -p please separate the changes in 2 sets, then cherry-pick one of those on a new branch created from 2.8.x, then we will merge it, and merge the result up in 2.9.x

@orklah orklah mentioned this pull request Apr 17, 2021
Merged
greg0ire
greg0ire approved these changes Apr 18, 2021
View reviewed changes
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

I learnt today that 2.9.0 was going to be released really soon, so there is no need to backport the "fixes" to 2.8.x

@greg0ire greg0ire merged commit b04d7a6 into doctrine:2.9.x Apr 18, 2021
@greg0ire
Copy link
Member

Thanks @orklah !

@greg0ire greg0ire added this to the 2.9.0 milestone Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire approved these changes

Assignees
No one assigned
Labels
Improvement
Projects
None yet
Milestone
2.9.0
Development

Successfully merging this pull request may close these issues.
2 participants
@orklah @greg0ire