loophp / collection

A (memory) friendly, easy, lazy and modular collection class.
https://loophp-collection.rtfd.io/
MIT License
721 stars 35 forks source link

Adds tap() - allows shorter syntax for chaining no side effects operations #340

Closed ddebowczyk closed 6 months ago

ddebowczyk commented 6 months ago

More convenient than apply() for chaining no side effect operations like logging or event dispatching. Allows for shorter notation:

$collection->tap(fn($item) => Event::dispatch(new ItemReceived($item)))

versus

$collection->apply(function($item) {
   Event::dispatch(new ItemReceived($item));
   return true;
})

This PR:

what-the-diff[bot] commented 6 months ago

PR Summary

drupol commented 6 months ago

Hi,

Thanks for the PR, it LGTM ! I wasn't aware that apply could be a burden to use and I don't mind adding tap into it. I agree that the syntax is simpler.

However, let's make sure tests are passing ;)

Let me know if you need some help.

ddebowczyk commented 6 months ago

Frankly, i just copied tests from apply and removed irrelevant fragment relying on boolean results. Not sure why they don't pass - any idea?

W dniu wt., 2.04.2024 o 08:06 Pol Dellaiera @.***> napisał(a):

Hi,

Thanks for the PR, it LGTM ! I wasn't aware that apply could be a burden to use and I don't mind adding tap into it. I agree that the syntax is simpler.

However, let's make sure tests are passing ;)

Let me know if you need some help.

— Reply to this email directly, view it on GitHub https://github.com/loophp/collection/pull/340#issuecomment-2031144257, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABM6RJW3K3JCK2IMQYDGIDY3JDGJAVCNFSM6AAAAABFSW7JOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZRGE2DIMRVG4 . You are receiving this because you authored the thread.Message ID: @.***>

drupol commented 6 months ago

There's a fundamental difference between apply and tap. Of course, they can do the same things, but since apply must returns a boolean, it can have a different way of working than tap... I guess the tests you've implemented are not reflecting that.

Also, it seems that there's an issue with the syntax.

To fix the code style:

./vendor/bin/grumphp run --tasks=phpcsfixer

To run the unit tests:

./vendor/bin/grumphp run --tasks=phpunit

To run the static analysis tests:

./vendor/bin/grumphp run --tasks=psalm.phpstan
ddebowczyk commented 6 months ago

I cannot install pcov on my machine currently, so my composer install fails = I can't execute code checks, tests, etc on my local env. For the time being I will continue using my fork. I will try getting back to it when I find some time to set everything up properly. Thanks for quick replies.

drupol commented 6 months ago

I understand. I wish I could help more concretely but right now, I'm busy on some other projects sadly.

If I can give a piece of advice to have an environment ready to use to develop in loophp/collection is to install nix (more info at https://zero-to-nix.com/):

curl --proto '=https' --tlsv1.2 -sSf -L https://install.determinate.systems/nix | sh -s -- install

To make it short, Nix is a universal package manager working on any Linux and Mac.

Once it is installed, go in the loophp/collection directory and do:

nix shell github:loophp/nix-shell#env-php81 --impure

This will take 2 to 3 minutes. Once it's done, a new temporary shell will be created where you'll have PHP 8.1 running with PCOV and everything you need to contribute to loophp/collection locally without interfering with your local PHP installation (if any). No container will be involved.

To exit the shell, just type exit and you're done.

ddebowczyk commented 6 months ago

Thanks again for the suggestion. I will get back with the fixes when I get tests running locally.

drupol commented 6 months ago

That was fast! Did you use Nix in the end?

ddebowczyk commented 6 months ago

Yes, Nix did the job. Thanks for suggesting it, saved me probably hours of install/config time.

drupol commented 6 months ago

Excellent, hope you'll use it for some other stuff too !!!

drupol commented 6 months ago

This feature is available in the newly released version 7.6.0.