-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat: Add Find
operation
#204
feat: Add Find
operation
#204
Conversation
Hey hi ! That's a nice start! I'm going to review it carefully today in the evening :) |
Could you give me write access to the repo so I can push the code-review changes? I pushed my local branch here: https://github.com/loophp/collection/tree/feature/add-find-helper but I prefer to push my changes on your branch, so you can continue to push things if needed. I did some changes in your |
Ah, of course. For some reason GitHub won't allow access to PR branches for org repos. I'll migrate it to my personal one. |
Yeah I'm trying to avoid repeating the same code all over the place. Using simple operations allows you to "compose" a new operation with a custom behavior without rewriting anything (almost). |
I just added 4 commits, it should fix the implementation and the code-style. Let me know if you plan to continue your work on this. Thanks for your contribution! |
2e54861
to
4567b33
Compare
No, I'm done. Thank you! |
Do you think you could provide the changes in For the rest, I'll do it myself if you don't have time. |
Thanks, much appreciated! |
I hope its okay style-wise - I'm not happy with the examples, it's hard to come up with something not too contrived, |
The code style can be fixed automatically by doing: I quickly checked the example file and I think it's nice, it definitely shows how this WDYT @AlexandruGG ? |
Overall looks good, there's a few things to improve on the documentation and tests side but we can make the additions. Do you want me to take care of it @drupol? |
As you wish, if you have time to do it, go ahead, or else I'll take care myself next week. |
Happy to do it, leave it with me :) |
- variable and param naming consistency - default value - update operation typing
- add static analysis checks - fix typing - add more unit tests
tests/static-analysis/find.php
Outdated
/* | ||
PHP 8 - using named parameters and the default `null` value -> these should legitimately fail, | ||
but Psalm reports no issue and PHPStan is reporting a different error than expected: | ||
"Parameter #1 $value of function find_checkNullableInt expects int|null, (Closure)|int given." | ||
|
||
find_checkNullableInt(Collection::fromIterable([1, 2, 3])->find(callbacks: $intValueCallback)); | ||
find_checkNullableString(Collection::fromIterable(['foo' => 'a', 'bar' => 'b'])->find(callbacks: $stringValueCallback)); | ||
*/ | ||
|
||
/* | ||
PHP 8 - using named parameters and the default `null` value -> these should legitimately fail, | ||
but Psalm reports no issue and the current PHPStan failures are due to the error mentioned above. | ||
|
||
find_checkIntElement(Collection::fromIterable([1, 2, 3])->find(callbacks: $intValueCallback)); | ||
find_checkStringElement(Collection::fromIterable(['foo' => 'a', 'bar' => 'b'])->find(callbacks: $stringValueCallback)); | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put these here for info, but we cannot turn them on because the CI checks will fail on 7.4.
The issue reported by PHPstan is weird; it might be a bug but I can't be bothered to reproduce it in the playground right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for providing the documentation and examples!
💯 ! |
@drupol this is ready for review! @Radiergummi thank you for the contribution, have a look at the latest code and let me know if everything's as you expected :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the commits and I'm ok with everything! Thanks for spotting all the things!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Radiergummi this is ready to go. If you have any comments let me know, otherwise I will merge it tomorrow |
I guess we can merge... |
Yup, LGTM. Thank you so much! :) |
@Radiergummi @drupol thank you both for the work on this! |
This PR:
find()
operationFixes #198.