Improve Imagick method types #2325
Conversation
resources/functionMap.php
Outdated
| 'Imagick::autoGammaImage' => ['bool', 'channel='=>'int'], | ||
| 'Imagick::autoLevelImage' => ['bool', 'CHANNEL='=>'string'], | ||
| 'Imagick::autoGammaImage' => ['bool', 'channel='=>'Imagick::CHANNEL_*'], | ||
| 'Imagick::autoLevelImage' => ['bool', 'channel='=>'Imagick::CHANNEL_*'], |
There was a problem hiding this comment.
int is correct. https://www.php.net/manual/en/imagick.autolevelimage.php
resources/functionMap.php
Outdated
| 'Imagick::compareImageLayers' => ['Imagick', 'method'=>'int'], | ||
| 'Imagick::compareImages' => ['array', 'compare'=>'imagick', 'metric'=>'int'], | ||
| 'Imagick::compositeImage' => ['bool', 'composite_object'=>'imagick', 'composite'=>'int', 'x'=>'int', 'y'=>'int', 'channel='=>'int'], | ||
| 'Imagick::compareImageChannels' => ['array{Imagick,float}', 'image'=>'imagick', 'channeltype'=>'Imagick::CHANNEL_*', 'metrictype'=>'Imagick::METRIC_*'], |
There was a problem hiding this comment.
| 'Imagick::getHDRIEnabled' => ['int'], | ||
| 'Imagick::getHomeURL' => ['string'], | ||
| 'Imagick::getImage' => ['Imagick'], | ||
| 'Imagick::getImageAlphaChannel' => ['int'], | ||
| 'Imagick::getImageAlphaChannel' => ['bool'], |
There was a problem hiding this comment.
https://www.php.net/manual/en/imagick.getimagealphachannel.php says:
imagick 3.6.0 Returns a bool now; previously, an int was returned.
| 'Imagick::getImageWidth' => ['int'], | ||
| 'Imagick::getInterlaceScheme' => ['int'], | ||
| 'Imagick::getImageWhitePoint' => ['array{x:float,y:float}'], | ||
| 'Imagick::getImageWidth' => ['0|positive-int'], |
There was a problem hiding this comment.
Oddly, getImageWidth is declared as unsigned long width, but getImageHeight seems to be just long height.
| 'ImagickKernel::separate' => ['array'], | ||
| 'ImagickKernel::seperate' => ['void'], |
resources/functionMap.php
Outdated
| @@ -5156,12 +5156,11 @@ | |||
| 'ImagickDraw::translate' => ['bool', 'x'=>'float', 'y'=>'float'], | |||
| 'ImagickKernel::addKernel' => ['void', 'ImagickKernel'=>'ImagickKernel'], | |||
| 'ImagickKernel::addUnityKernel' => ['void'], | |||
| 'ImagickKernel::fromBuiltin' => ['ImagickKernel', 'kernelType'=>'string', 'kernelString'=>'string'], | |||
| 'ImagickKernel::fromBuiltin' => ['ImagickKernel', 'kernelType'=>'Imagick::KERNEL_*', 'kernelString'=>'string'], | |||
There was a problem hiding this comment.
resources/functionMap.php
Outdated
| 'Imagick::adaptiveResizeImage' => ['bool', 'columns'=>'int', 'rows'=>'int', 'bestfit='=>'bool'], | ||
| 'Imagick::adaptiveSharpenImage' => ['bool', 'radius'=>'float', 'sigma'=>'float', 'channel='=>'int'], | ||
| 'Imagick::adaptiveSharpenImage' => ['bool', 'radius'=>'float', 'sigma'=>'float', 'channel='=>'Imagick::CHANNEL_*'], |
There was a problem hiding this comment.
I'm not a big fan of narrowing down parameter types. It requires people to suddently start typehinting PHPStan-specific types in their PHPDocs instead of int. For the same reason I haven't merged #2163 and #2271 yet.
But I think it'd be fine to introduce these typehints in bleedingEdge/PHPStan 2.0.
I've added support for bleedingEdge-specific functionMap: 06b746d
My suggestion is:
- Submit a PR that only changes return types for everyone.
- Submit a next PR that changes parameter types in
functionMap_bleedingEdge.php.
Thank you!
There was a problem hiding this comment.
I'm not a big fan of narrowing down parameter types.
I agree with your policy because it makes sense. Thank you for your review!
|
Thank you! |
Follows
Imagickconstants and detailedarraytypes.