Skip to content

Conversation

@yjv
Copy link

@yjv yjv commented Dec 28, 2015

adds an adapter that allows you to define a callback to be applied to all items before theyre given to the paginator

@Koc
Copy link

Koc commented Dec 29, 2015

@yjv very useful, we using similar behavior in one of our adapters https://github.com/Koc/Sphinxy/blob/master/Pagerfanta/Adapter/SphinxyQbAdapter.php#L33 . But I have some proposals:

  1. IMHO better name it with FilteringAdapter or WrappingAdapter
  2. What about call it once for slice instead of iterating slice and call for each item? This has next benefits:
    2.1. we can access to keys of the slice array
    2.2. slice will not converted into array (it can be object)

Copy link

Choose a reason for hiding this comment

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

@param callable $callback

… used to pass it the entire slice instead of each item to allow for seeing keys and not forcing the slice into an array
@yjv
Copy link
Author

yjv commented Dec 29, 2015

@Koc thanks for all the feedback!

concerning your first point im naming it after how php names that functoinality itself. along those lines one named filter adapter i would think would filter out items not jsut map their values. naming it WrappingAdapter doesnt seem to me to tell what makes it special over any other adapter that wraps another adapter and does things with it. an example of an adapter that i think would also fall under the name wrapping adapter would be the AdaptersAdapter I have put in the other PR recently. So i figured a name a little more specific to what this one is doing ie mapping an item to another was more appropriate.

for your second point, I agree and ive made the changes to match!

thanks :)

README.md Outdated
Copy link

Choose a reason for hiding this comment

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

Docs should be updated after your last changes

@yjv
Copy link
Author

yjv commented Dec 30, 2015

@Koc @stof changes made.

Thanks!

@yjv
Copy link
Author

yjv commented Jan 10, 2016

@Koc @stof is this good to be merged?

thanks

@Koc
Copy link

Koc commented Jan 10, 2016

👍 from me, but I haven't rights to merge anything to this repo.

ping @richsage @pablodip

README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

typo here: callback t => callback to

@richsage
Copy link
Contributor

@yjv I've just added a couple more comments - nothing major though :)

@pablodip
Copy link
Contributor

How about mapping inside the adapter, and not outside, like in tests? If mapping is done inside, then it is a mapping adapter, but if it it done out side it is just a callback adapter.

@yjv
Copy link
Author

yjv commented Jan 11, 2016

@pablodip yeah good point. For some reason i didn't notice the CallbackAdapter until you pointed it out.

I changed it to that when it was pointed out to me that it would force all slices to be arrays in the end so even if the inner adapter returned an iterator the mapping adapter would return an array since it would have to get each value and map it.

After seeing that there is already a CallbackAdapter i agree that yeah unless this one applies the callback to each element alone its not useful enough. I could make the mapping adapter always loop through and return an array no matter what the inner adapter returns. I could also make or pull in a mapping iterator in the case of a traversable returned and call array_map when an array is returned from the inner iterator.

What are your thoughts?

Thanks!

@yjv
Copy link
Author

yjv commented Jan 11, 2016

@richsage made fixes.

thanks!

@pablodip
Copy link
Contributor

The current CallbackAdapter has a different behaviour, since it doesn't delegate to another adapter.

I'd probably go for an adapter open to composability, so that you could do mapping or whatever you wish. Something like TransformingSliceAdapter, where you could transform the slice the way you want. On op of that a MappingAdapter could be created to ease mapping.

@stof
Copy link
Contributor

stof commented Mar 23, 2016

The current CallbackAdapter has a different behaviour, since it doesn't delegate to another adapter

That's not hard to do though:

$innerAdapter = //...

$adapter = new CallbackAdapter(
    function () use ($innerAdapter) {
        return $innerAdapter->getNbResults();
    },
    function ($offset, $length) use ($innerAdapter) {
        $data = $innerAdapter->getSlice($offset, $length);

        // Transform data

        return $data;
    }
));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants