Scalar and return type declarations + more #12
Conversation
src/Image.php Outdated
| * @method Image flip($direction, array $options = []) | ||
| * @method Image rot($angle, array $options = []) | ||
| * @method static Image black(int $width, int $height, array $options = []) | ||
| * @method static Image linear(float $a, float $b, array $options = []) |
There was a problem hiding this comment.
I'm planning to generate most of these automatically from introspection, I wouldn't spend too much time writing them by hand. For example, the Ruby binding has this file:
https://github.com/jcupitt/ruby-vips/blob/master/lib/vips/methods.rb
Generated automatically by this code:
https://github.com/jcupitt/ruby-vips/blob/master/lib/vips/image.rb#L1282
It should be easy to adapt to PHP.
There was a problem hiding this comment.
Oh, it should be possible to generate the constants automatically too, I'd leave those.
There was a problem hiding this comment.
I had a quick go, see: #8
https://github.com/jcupitt/php-vips/blob/dev/examples/docs.php
There was a problem hiding this comment.
Nice! Removed mine in the new commit (writing them by hand is hard and time inefficient.)
There was a problem hiding this comment.
And makes maintenance harder going forward :( Yes, we should auto-generate as much as we can, I think.
src/Image.php Outdated
| * @param Image $match_image Use this image as a guide. | ||
| * @param mixed $value Turn this into an image. | ||
| * @param Image $match_image Use this image as a guide. | ||
| * @param mixed $value Turn this into an image. |
There was a problem hiding this comment.
phpcs insists on this strange indenting, we probably need to keep it.
There was a problem hiding this comment.
Fixed in the new commit.
src/Image.php Outdated
| */ | ||
| | ||
| $arguments = array_merge([$name, $instance], $arguments); | ||
| $arguments = array_merge([$name, $instance], $options, $arguments); |
There was a problem hiding this comment.
I'm getting confused :( Do we really want to make self::call into a varargs function?
How about separating options out as a separate argument, so it's
call(string $name, $instance, array $arguments, array $options = []) : array Now when we call it from a method, we can do:
return self::call($base . "_const", $this, [$op, $other], $options); And avoid the array_merge(), at least some of the time.
There was a problem hiding this comment.
You're right, we don't need to make self::call into a varargs function! I've fixed it in the new commit.
Sorry about the the confusing.
kleisauke force-pushed the scalar-return-type-declarations-and-phpdoc-fix branch from 6160d29 to 457ffb5 Compare 
| Are we ready to merge to dev, do you think? We can leave the tests for another pull request. |
src/Image.php Outdated
| } | ||
| | ||
| // TODO Unused variable | ||
| $height = count($value); |
There was a problem hiding this comment.
This variable is still unused. What should we do with it?
There was a problem hiding this comment.
Ooops, yes, let's remove it. It was legft over from an earlier edit.
| * @param array $options An array of options to pass to the operation. | ||
| * | ||
| * @return mixed The result(s) of the operation. | ||
| * @return mixed The result(s) of the operation. |
There was a problem hiding this comment.
mixed -> Image|array? Or are the more types that vips_call can return?
There was a problem hiding this comment.
Yes, Image::call can return numbers (min() for example), strings, arrays ... lots of stuff.
src/Image.php Outdated
| * @param array $options An array of options to pass to the operation. | ||
| * | ||
| * @return mixed[] The operation result. | ||
| * @return Image The operation result. |
There was a problem hiding this comment.
Can _callEnum return a other type than Image? Because call returns mixed.
There was a problem hiding this comment.
It'll always be Image at the moment, but it could potentially return anything. It should probaly be mixed to match Vips::call.
| @jcupitt I think we are ready to merge this to dev. Added a few inline comments. |
- Added scalar & return type declarations - Fixed some issues with `array_merge` (libvips#11). - Fixed return and scalar PHPDoc values (long -> int, double -> float, mixed[] -> array). And edited some to the right ones. - Updated PHPUnit. - Corrected some typos. - phpcs indenting fixes.
kleisauke force-pushed the scalar-return-type-declarations-and-phpdoc-fix branch from 457ffb5 to 8ed79bd Compare kleisauke changed the title | @jcupitt |
| OK, merged! Nice work Kleis! |
2 participants
Made a beginning with property and method tags (document magic methods #8).writing them by hand is hard and time inefficient.array_merge(ifthenelse error when $options parameter is given #11).TODO:
OK (27 tests, 64 assertions)).VIPS_INTERPRETATION,VIPS_ANGLEetc.).