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

add mapping adapter #195

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

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)

/**
* MappingAdapter constructor.
* @param AdapterInterface $innerAdapter
* @param $callback
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 :)

$results = array(/* ... */);

$adapter = new MappingAdapter(new ArrayAdapter($results), function ($item) {
return $item + 1;
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

@@ -333,6 +333,26 @@ $results = array(/* ... */);
$adapter = new FixedAdapter($nbResults, $results);
```

### MappingAdapter

This adapter allows you to define a callback t be applied to all items coming out of the adapter.
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