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

Support generics in docblock #3544

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

tm1000
Copy link

@tm1000 tm1000 commented Feb 8, 2022

This supports the following:

    /**
	 * @param Collection<int, TelephoneNumber> $telephoneNumbers
	 */
	public function myFunction(
		Collection $telephoneNumbers,

Where-as before it would say that the typehint needs to be Collection<int, TelephoneNumber> now it will correctly deduce that the type hint needs to be Collection and will not return an error

I based this off of #708 which is similar in scope.

Unfortunately I was unable to run the unit tests because I'm on PHP 8.1 and something in the code continues to try to use the function each which does not exist in php 8.0

Edit: There is now a test!

@jrfnl
Copy link
Contributor

jrfnl commented Feb 8, 2022

Unfortunately I was unable to run the unit tests because I'm on PHP 8.1 and something in the code continues to try to use the function each which does not exist in php 8.0

Please add tests for this feature.

To run the tests locally on PHP 8.1:

  • Run composer install --ignore-platform-reqs to get PHPUnit 7.5.
  • Then run vendor/bin/phpunit tests/AllTests.php --no-configuration --bootstrap=tests/bootstrap.php --dont-report-useless-tests

See:

# For PHP 8.0+, we need to install with ignore platform reqs as PHPUnit 7 is still used.
- name: Install Composer dependencies - with ignore platform
if: ${{ matrix.php >= '8.0' }}
uses: "ramsey/composer-install@v1"
with:
composer-options: --ignore-platform-reqs
# Note: The code style check is run multiple times against every PHP version
# as it also acts as an integration test.
- name: 'PHPCS: set the path to PHP'
run: php bin/phpcs --config-set php_path php
- name: 'PHPUnit: run the tests'
if: ${{ matrix.php != '8.1' && matrix.php != '8.2' }}
run: vendor/bin/phpunit tests/AllTests.php
# We need to ignore the config file so that PHPUnit doesn't try to read it.
# The config file causes an error on PHP 8.1+ with PHPunit 7, but it's not needed here anyway
# as we can pass all required settings in the phpunit command.
- name: 'PHPUnit: run the tests on PHP > 8.0'
if: ${{ matrix.php == '8.1' || matrix.php == '8.2' }}
run: vendor/bin/phpunit tests/AllTests.php --no-configuration --bootstrap=tests/bootstrap.php --dont-report-useless-tests

@tm1000
Copy link
Author

tm1000 commented Feb 8, 2022

@jrfnl got it to work! I had to delete the composer.lock file and vendor dir then run with --ignore-platform-reqs

I deleted my other comment because I was stuck on phpunit 4 because I didnt run with --ignore-platform-reqs

Thanks for pointing that out. much appreciated. There is now a test!

Copy link
Contributor

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Looks good, just two small notes.

@@ -419,7 +419,9 @@ protected function processParams(File $phpcsFile, $stackPtr, $commentStart)

// Check type hint for array and custom type.
$suggestedTypeHint = '';
if (strpos($suggestedName, 'array') !== false || substr($suggestedName, -2) === '[]') {
if (preg_match('/^(.*)<.*>$/', $suggestedName, $matches)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. For performance optimization, I'd recommend having this condition as the last condition in the if/elseif chain, not the first.
  2. I'd also suggest optimizing the regex a little more, like so:
    Suggested change
    if (preg_match('/^(.*)<.*>$/', $suggestedName, $matches)) {
    if (preg_match('/^([^<]+)<[^>]+>$/', $suggestedName, $matches)) {

*/
public function specifiedArray1(Collection $values) {

}// end specifiedArray1()
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new line at end of file (also for fixed).

@tm1000
Copy link
Author

tm1000 commented Feb 10, 2022

@jrfnl Did as you asked however I could not make the if/then last. I was able to make it second to last. The reasoning is because the last if/then is a "if not in list then do this". The suggestedName before the expression will never be in that array, thus the last if/then would always match

@jrfnl
Copy link
Contributor

jrfnl commented May 16, 2022

Linking issue #3589 for visibility.

@tm1000
Copy link
Author

tm1000 commented Nov 7, 2022

@jrfnl @gsherwood Now that its been about 9 months what is the process to get this merged?

@jrfnl
Copy link
Contributor

jrfnl commented Nov 8, 2022

@tm1000 Probably not what you want to hear, but patiently wait until Greg has some time to review this. I can review (which I did), but I don't have commit, so there's nothing more I can do.

@WillemHoman
Copy link

@jrfnl would love to see this merged, is that going to be possible?

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.

None yet

4 participants