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

Feat: null filter #25

Merged
merged 1 commit into from
Feb 11, 2021
Merged

Feat: null filter #25

merged 1 commit into from
Feb 11, 2021

Conversation

bpolaszek
Copy link
Owner

Since we can omit the 2nd argument of the native array_filter function, resulting in an output array is filtered on truthy values, we should be able to invoke IterableObject::filter() similarly.

This PR allows IterableObject::filter() to accept a null callback (or no args at all) to enable this behavior.

To prevent this default filter from being applied when IterableObject::$filter is null (with the "no filter" intention), IterableObject's constructor is now private, and the IterableObject::filter() method MUST be used to ensure user's intention. Hence $filter and $map arguments from the iterable() function have been removed to enforce using the fluent interface.

@simPod would you mind reviewing this?

@bpolaszek bpolaszek self-assigned this Feb 4, 2021
@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #25 (fd05360) into 2.0.x-dev (7da8363) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           2.0.x-dev       #25   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
===========================================
  Files              2         2           
  Lines             43        48    +5     
===========================================
+ Hits              43        48    +5     
Impacted Files Coverage Δ
src/IterableObject.php 100.00% <100.00%> (ø)
src/iterable-functions.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7da8363...fd05360. Read the comment docs.

@simPod
Copy link
Contributor

simPod commented Feb 4, 2021

Today I stumbled upon this iterable() fn wondering what it does, had no time to check the docs so I guess the time is now 😄

But first, I don't get the readme

When you have an iterable type-hint somewhere, and don't know in advance wether you'll pass an array or a Traversable, just call the magic iterable() factory:

Why should it matter whether I pass an array or Traversable into iterable?

@bpolaszek
Copy link
Owner Author

bpolaszek commented Feb 5, 2021

Why should it matter whether I pass an array or Traversable into iterable?

I think there's a mistake in the doc indeed ^^ I thought about iterable output, array|Traversable input. Example:

interface StuffProviderInterface
{
    public function getStuff(): iterable;
}

function ingestStuff(Traversable $stuff): void
{
    // do something
}

// ...

ingestStuff(iterable($stuffProvider->getStuff())); // bec. we're not supposed to know if the implementation returns an array or a Traversable

Similarly:

interface StuffProviderInterface
{
    public function getStuff(): iterable;
}

function ingestStuff(array $stuff): void
{
    // do something
}

// ...

ingestStuff(iterable($stuffProvider->getStuff())->asArray());

But that's not really relevant since we provide functions for that.

@bpolaszek
Copy link
Owner Author

bpolaszek commented Feb 5, 2021

But basically we should think about iterable as a convenient, fluent factory for IterableObject (which is @internal and is not supposed to be instanciated outside of the library) to write code like iterable($stuff)->filter($filterFn)->map($mapFn). I will rewrite that section of the doc.


public function filter(?callable $filter = null): self
{
$filter = $filter ?? function ($value): bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

When I did the benchmarking last time on 7.4, using intval or boolval in this case was faster than anon. fcn.

Copy link
Owner Author

@bpolaszek bpolaszek Feb 6, 2021

Choose a reason for hiding this comment

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

Good idea 🙂 but I just tested it, and it throws warnings when used within CallbackFilterIterator because it expects 1 parameter, and the CallbackFilterIterator injects 3.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That reminds me that we currently don't support the $mode argument of the array_filter() function, I'll try to address that in a separate PR.

@bpolaszek
Copy link
Owner Author

@simPod I'd like to merge this one, along with #26. Any objection?

@simPod
Copy link
Contributor

simPod commented Feb 11, 2021

@bpolaszek no, let's 🚢

@bpolaszek bpolaszek merged commit 4c46851 into 2.0.x-dev Feb 11, 2021
@bpolaszek bpolaszek deleted the feature/null-filter branch February 11, 2021 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants