Skip to content

Improve Imagick method types#2325

Merged
ondrejmirtes merged 3 commits intophpstan:1.10.xfrom
zonuexe:types/imagick
Apr 5, 2023
Merged

Improve Imagick method types#2325
ondrejmirtes merged 3 commits intophpstan:1.10.xfrom
zonuexe:types/imagick

Conversation

@zonuexe
Copy link
Copy Markdown
Contributor

@zonuexe zonuexe commented Apr 4, 2023

Follows Imagick constants and detailed array types.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oddly, getImageWidth is declared as unsigned long width, but getImageHeight seems to be just long height.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

typo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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:

  1. Submit a PR that only changes return types for everyone.
  2. Submit a next PR that changes parameter types in functionMap_bleedingEdge.php.

Thank you!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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!

@ondrejmirtes ondrejmirtes merged commit 82fa54b into phpstan:1.10.x Apr 5, 2023
@ondrejmirtes
Copy link
Copy Markdown
Member

Thank you!

@zonuexe zonuexe deleted the types/imagick branch April 5, 2023 11:02
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.

2 participants