-
Notifications
You must be signed in to change notification settings - Fork 2
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
Concatenation adapter #239
Concatenation adapter #239
Conversation
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.
This is a great PR. Just a handful of minor changes please. Thanks again for contributing.
foreach ($adapters as $index => $adapter) { | ||
if (!($adapter instanceof AdapterInterface)) { | ||
throw new InvalidArgumentException( | ||
'Argument $adapters['.$index.'] expected to be a \Pagerfanta\Adapter\AdapterInterface instance' |
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.
This is quite hard to read. Please could you use sprintf
instead?
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.
✅ Is it good now?
$fetchOffset = $searchFirstIndex - $adapterFirstIndex; | ||
$fetchLength = $length; | ||
|
||
// The requested range peeps above the bottom edge of the adapter range |
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.
Could you update this comment to be The requested range peeps below the bottom edge of the adapter range
? (above
to below
). That would be clearer.
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.
@sampart You are right. The comment describes the following situation:
0 1 2 3 4 5 6 7 8 9 10
Current adapter range: | [4 | 9]
Requested range: [2 6]
✅ I've changed the comments and the variables names to make the code more clear.
use Pagerfanta\Adapter\FixedAdapter; | ||
use Pagerfanta\Adapter\NullAdapter; | ||
|
||
class ConcatenationAdapterTest extends \PHPUnit_Framework_TestCase |
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.
Since you submitted this PR, we've merged #240, which uses namespaced PHPUnit files. Please could you change this line to be class ConcatenationAdapterTest extends TestCase
and add the following use statement?
use PHPUnit\Framework\TestCase;
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.
✅
…concatenation-adapter
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.
Looks great, thanks!
@sampart I’m glad to read your feedback. Thank you! |
@sampart And thank you for supporting Pagerfanta, it’s a great library. |
Thanks for the kind words, @FinesseRus (and for your contributions!) |
I've made a new adapter. It joins the results of adapter instances into a single adapter.