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

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

Merged
merged 4 commits into from Apr 2, 2024

Conversation

ddebowczyk
Copy link
Contributor

@ddebowczyk ddebowczyk commented Apr 2, 2024

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:

  • [+] Provides shorter notation for no side effect operations
  • [-] It breaks backward compatibility
  • [+] Is covered by unit tests
  • [+] Has static analysis tests (psalm, phpstan)
  • [+] Has documentation
  • [-] Is an experimental thing

@ddebowczyk ddebowczyk requested a review from drupol as a code owner April 2, 2024 05:58
Copy link

what-the-diff bot commented Apr 2, 2024

PR Summary

  • New 'tap' operation added to the Collection class
    Our software's Collection class got an enhancement with a new operation called 'tap'. Its corresponding agreement (also known as a 'contract') and its implementation have also been added.

  • New file additions related to 'tap' operation
    Three new files are added in various directories related to the new 'tap' operation. These are 'Tappable.php' in the 'Contract/Operation' directory, 'Tap.php' in the 'Operation' directory, and 'tap.php' in the 'tests/static-analysis' directory.

  • New test case for 'tap' operation
    To ensure that the new 'tap' operation works properly, a new test case has been added to CollectionSpecificOperationTest.php. It helps us to confirm that the new feature behaves as expected.

@drupol
Copy link
Collaborator

drupol commented Apr 2, 2024

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
Copy link
Contributor Author

ddebowczyk commented Apr 2, 2024 via email

@drupol
Copy link
Collaborator

drupol commented Apr 2, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

drupol commented Apr 2, 2024

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.

@drupol drupol added the enhancement New feature or request label Apr 2, 2024
@ddebowczyk
Copy link
Contributor Author

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

@drupol
Copy link
Collaborator

drupol commented Apr 2, 2024

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

@ddebowczyk
Copy link
Contributor Author

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

@drupol
Copy link
Collaborator

drupol commented Apr 2, 2024

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

Copy link
Collaborator

@drupol drupol left a comment

Choose a reason for hiding this comment

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

LGTM, I'll merge it as it is and maybe do some optimisations later on.

Thank you very much for your contribution!

@drupol drupol merged commit 817729e into loophp:master Apr 2, 2024
15 checks passed
@drupol
Copy link
Collaborator

drupol commented Apr 2, 2024

This feature is available in the newly released version 7.6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants