Skip to content

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Oct 10, 2022

just playing around, kind of POC for moving more array-related methods to Type which I mentioned in #1801 (comment). I'm not sure about this or if this should be the direction to move forward for other array return type extensions too..

What I like

  • no special constant array handling and union type building anymore in ArrayPopFunctionReturnTypeExtension. I actually love that

What I dislike

  • it's basically just for constant array and array_pop in particular
  • boiler plate code in some type classes, but this is already minimized thanks to the new array traits
  • naming-wise it's maybe hard to understand in some cases, e.g. how would flip() be called on Type and what do people expect it to do ..
  • not much related to this PR, but there's also MaybeIterableTypeTrait and NonIterableTypeTrait which are kind of related to the new array type traits IMO

Most likely this is the best-case example and others will be more complicated to move over.

This PR is really mostly just for discussing this to not do this in other PRs that are not related :)

@herndlm herndlm force-pushed the last-iterable-value-type branch from 04e88a4 to 4e5af92 Compare October 10, 2022 15:27
@herndlm herndlm marked this pull request as ready for review October 10, 2022 15:40
@herndlm herndlm force-pushed the last-iterable-value-type branch 5 times, most recently from 2a79d0c to c4bb029 Compare October 10, 2022 15:50
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

This totally makes sense to me. Maybe we could tackle first value at the same time? Thanks 😊

@herndlm herndlm force-pushed the last-iterable-value-type branch 2 times, most recently from 1f88a93 to 5bffea3 Compare October 10, 2022 16:12
@ondrejmirtes
Copy link
Member

BTW a random thought - don't feel obligated to get the design right the first time, it doesn't have to be perfect 😊 We'll eventually deprecate some of the new methods anyway, as we come up with better alternatives 😊

@herndlm herndlm requested a review from ondrejmirtes October 10, 2022 16:34
@herndlm
Copy link
Contributor Author

herndlm commented Oct 10, 2022

BTW a random thought - don't feel obligated to get the design right the first time, it doesn't have to be perfect 😊 We'll eventually deprecate some of the new methods anyway, as we come up with better alternatives 😊

Yes. But you know, it is very important that they are sorted in the right order 😅

@herndlm herndlm force-pushed the last-iterable-value-type branch 2 times, most recently from 5f3f89a to 594e613 Compare October 10, 2022 17:32
@herndlm herndlm force-pushed the last-iterable-value-type branch from 594e613 to 401b2f5 Compare October 10, 2022 17:32
@herndlm
Copy link
Contributor Author

herndlm commented Oct 10, 2022

I saw a couple more cases like this one, expect more PRs :) I'll look at the complex ones at the end

@herndlm
Copy link
Contributor Author

herndlm commented Oct 11, 2022

this is ready and I'm eager to add a couple more 😊

@ondrejmirtes ondrejmirtes merged commit c203f6a into phpstan:1.9.x Oct 11, 2022
@ondrejmirtes
Copy link
Member

Thank you :)

@herndlm herndlm deleted the last-iterable-value-type branch October 11, 2022 13:46
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