-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
base: master
Are you sure you want to change the base?
Conversation
Please add tests for this feature. To run the tests locally on PHP 8.1:
See: PHP_CodeSniffer/.github/workflows/test.yml Lines 121 to 142 in 3e6ce10
|
@jrfnl got it to work! I had to delete the composer.lock file and vendor dir then run with I deleted my other comment because I was stuck on phpunit 4 because I didnt run with Thanks for pointing that out. much appreciated. There is now a test! |
There was a problem hiding this 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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For performance optimization, I'd recommend having this condition as the last condition in the if/elseif chain, not the first.
- 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() |
There was a problem hiding this comment.
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).
@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 |
Linking issue #3589 for visibility. |
@jrfnl @gsherwood Now that its been about 9 months what is the process to get this merged? |
@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. |
…Sniffer into feature/support-generics
@jrfnl would love to see this merged, is that going to be possible? |
This supports the following:
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 beCollection
and will not return an errorI 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 functioneach
which does not exist in php 8.0Edit: There is now a test!