Skip to content
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

refactor: unfold constructor is not variadic anymore #269

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Nov 3, 2022

This PR:

  • Fix ...
  • Provide ...
  • It breaks backward compatibility
  • Is covered by unit tests
  • Has static analysis tests (psalm, phpstan)
  • Has documentation
  • Is an experimental thing

Follows #. Related to #. Fixes #.

@what-the-diff
Copy link

what-the-diff bot commented Nov 3, 2022

  • The unfold operation now accepts an iterable instead of a variadic list.
  • The signature for the unfold callback has been changed to accept either T or U, where U is the type in $parameters (see tests/static-analysis/unfold.php).

@drupol drupol force-pushed the refactor/unfold-constructor branch 2 times, most recently from 335b3ec to e742c22 Compare November 3, 2022 12:31
@drupol drupol marked this pull request as ready for review November 3, 2022 12:45
Comment on lines 22 to 23
* @param callable((T|U) ...): list<T> $callback
* @param array<int, U> $parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

so does this mean that now in $parameters we want to be able to pass values of different types? What would be a good use case for that?

Copy link
Collaborator Author

@drupol drupol Nov 3, 2022

Choose a reason for hiding this comment

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

Yes we could potentially do that, but T might also be the same as U.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I gave a second look yesterday and you are perfectly right. I reverted some of the changes.

It's ready again for review.

@drupol drupol force-pushed the refactor/unfold-constructor branch 2 times, most recently from 00af91e to 3628b2f Compare November 4, 2022 08:58
@drupol drupol force-pushed the refactor/unfold-constructor branch from 3628b2f to d424e71 Compare November 4, 2022 09:04
interface Unfoldable
{
/**
* Create a collection by yielding from a callback with an initial value.
*
* @see https://loophp-collection.readthedocs.io/en/stable/pages/api.html#unfold
*
* @template T
* @template U of T
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why this is needed - doesn't it work if we leave it like before, just T ? Either way I guess it's the same thing

Copy link
Collaborator Author

@drupol drupol Nov 4, 2022

Choose a reason for hiding this comment

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

Nope it doesn't work. This is a static function, so, U must be defined locally at the method level. And U is going to be made of T.

@drupol drupol merged commit d5988be into master Nov 4, 2022
@drupol drupol deleted the refactor/unfold-constructor branch November 4, 2022 09:11
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