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

Update Span and Partition operations. #148

Merged
merged 8 commits into from
Jul 17, 2021

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Jul 15, 2021

This PR:

@drupol drupol mentioned this pull request Jul 15, 2021
@drupol drupol added bug Something isn't working backward breaking change labels Jul 15, 2021
src/Collection.php Outdated Show resolved Hide resolved
@drupol drupol force-pushed the 147-fix-issue-with-iterable-iterator branch 3 times, most recently from e44d50b to 5cf143d Compare July 16, 2021 16:10
@drupol
Copy link
Collaborator Author

drupol commented Jul 16, 2021

@AlexandruGG Let me know what you think. I think this is clearly an improvement.

@drupol drupol force-pushed the 147-fix-issue-with-iterable-iterator branch from 5cf143d to 332c542 Compare July 16, 2021 17:24
@drupol drupol changed the title Fix issue with iterable iterator Update Span and Partition operations. Jul 16, 2021
AlexandruGG added 2 commits July 16, 2021 20:33
(cherry picked from commit 9eaf340)
(cherry picked from commit 5afe280)
@drupol drupol force-pushed the 147-fix-issue-with-iterable-iterator branch from 332c542 to ec3ecf9 Compare July 16, 2021 18:33
@drupol drupol force-pushed the 147-fix-issue-with-iterable-iterator branch from ec3ecf9 to ff9f05b Compare July 16, 2021 19:40
@drupol drupol marked this pull request as ready for review July 16, 2021 19:44
@AlexandruGG
Copy link
Collaborator

@drupol hey, so if I understand correctly the main issue is that fromCallable should be used whenever you want to instantiate the Collection from a Generator, right? And then it means that this example is kind of expected behaviour due to how PHP Generators work, right?

I'm thinking I can update the documentation to mention this and fix the static analysis as well

@drupol
Copy link
Collaborator Author

drupol commented Jul 17, 2021

Yes exactly, and it make sense actually, I don't know why I haven't though about this earlier.

@AlexandruGG
Copy link
Collaborator

Yes exactly, and it make sense actually, I don't know why I haven't though about this earlier.

Cool 😄. I thought about it at first but then I also was assuming that with Collection and ClosureIterator we want to still allow that. But it makes sense to keep it consistent with how a Generator would work by itself

@AlexandruGG
Copy link
Collaborator

Leave this to me, I'll fix the static analysis and update the docs

@drupol
Copy link
Collaborator Author

drupol commented Jul 17, 2021

Yes exactly, and it make sense actually, I don't know why I haven't though about this earlier.

Cool 😄. I thought about it at first but then I also was assuming that with Collection and ClosureIterator we want to still allow that. But it makes sense to keep it consistent with how a Generator would work by itself

As a closure is the real immutable thing in PHP, we must use and abuse them 💪

@drupol
Copy link
Collaborator Author

drupol commented Jul 17, 2021

Leave this to me, I'll fix the static analysis and update the docs

Thanks :) and ok, I don't touch this anymore.

@drupol
Copy link
Collaborator Author

drupol commented Jul 17, 2021

@drupol hey, so if I understand correctly the main issue is that fromCallable should be used whenever you want to instantiate the Collection from a Generator, right? And then it means that this example is kind of expected behaviour due to how PHP Generators work, right?

Yes ! We could imagine fixing it by detecting if the iterable is a Generator, and then wrap it in a CachingIterator. We should tackle that in another PR anyway, see #150.

.. tip:: This can be very useful when working with a PHP `Generator`_, since it will allow the collection
object to behave as if the Generator was rewindable.

Signature: ``Collection::fromCallable(callable $callable, iterable $parameters = []): Collection;``
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't forget to update these when changing method signatures 😬
#146

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops! Thanks!

AlexandruGG
AlexandruGG previously approved these changes Jul 17, 2021
Copy link
Collaborator

@AlexandruGG AlexandruGG left a comment

Choose a reason for hiding this comment

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

This should be good to go. Here's what I did:

  • updated docs to mention the Generator behaviour. We can further update once feat: Add ::fromGenerator constructor. #150 is ready
  • fixed static analysis - required being a bit more verbose in the Collection.php methods and in the operations but works
  • moved the new tests into the existing partition test

@drupol drupol merged commit a531675 into master Jul 17, 2021
@drupol drupol deleted the 147-fix-issue-with-iterable-iterator branch July 17, 2021 11:47
@drupol
Copy link
Collaborator Author

drupol commented Jul 17, 2021

A nice one done! And a good lesson learned :)

@drupol
Copy link
Collaborator Author

drupol commented Jul 17, 2021

Thanks for the help @AlexandruGG !

@AlexandruGG
Copy link
Collaborator

Thank you for fixing this 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants