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

Offset Paginator #31

Merged
merged 11 commits into from Feb 5, 2020
Merged

Conversation

@roxblnfk
Copy link
Member

roxblnfk commented Feb 1, 2020

Q A
Is bugfix? ✔️
New feature? ✔️
Breaks BC? ✔️ (some PaginatorInterface's methods now return self)
Tests pass? ✔️

Added OffsetPaginatorInterface. Because the OffsetPaginator is final.
Some changes in the PaginatorInterface. Not sure if this is a useful change.

OffsetPaginator refactored:

  • Cleanup and fixes
  • The Paginator now throws PaginatorException in read(), isOnLastPage() and getCurrentPageSize() methods when a CurentPage greater than a pages count
  • Removed data cache from the Paginator. Now caching should by in a DataReader
  • I also accept suggestions for renaming new methods (list of new methods you can see in the OffsetPaginatorInterface)
  • more tests
…Interface changed
roxblnfk added 3 commits Feb 3, 2020
…ething; added test cases
@roxblnfk roxblnfk marked this pull request as ready for review Feb 3, 2020
@roxblnfk roxblnfk requested review from yiisoft/reviewers and samdark Feb 3, 2020
src/Paginator/OffsetPaginator.php Outdated Show resolved Hide resolved
src/Paginator/OffsetPaginator.php Show resolved Hide resolved
src/Paginator/OffsetPaginator.php Outdated Show resolved Hide resolved

namespace Yiisoft\Data\Paginator;

interface OffsetPaginatorInterface extends PaginatorInterface

This comment has been minimized.

Copy link
@samdark

samdark Feb 3, 2020

Member

There's currently a single implementation, OffsetPaginator. What other implementations do you expect?

This comment has been minimized.

Copy link
@roxblnfk

roxblnfk Feb 3, 2020

Author Member

Any broader, but which will be compatible with our interface

This comment has been minimized.

Copy link
@samdark

samdark Feb 3, 2020

Member

We need to make sure there are real cases for implementing your own offset paginator before introducing an interface.

This comment has been minimized.

Copy link
@samdark

samdark Feb 3, 2020

Member

@yiisoft/reviewers would you please help us with ideas?

This comment has been minimized.

Copy link
@xepozz

xepozz Feb 4, 2020

Contributor

I fully support the @roxblnfk. Working with an interface in other components is better than working with a specific implementation.

Who will use it?
For example, creators of custom extensions (like kartik-v for yii2) or someone wants to make their own framework based on yii3 components.

This comment has been minimized.

Copy link
@roxblnfk

roxblnfk Feb 4, 2020

Author Member

But current PaginatorInterface have some methods that are not required for the OffsetPaginator:
withNextPageToken()
withPreviousPageToken()
getNextPageToken()
getPreviousPageToken()

This comment has been minimized.

Copy link
@kamarton

kamarton Feb 4, 2020

Contributor

But current PaginatorInterface have some methods that are not required for the OffsetPaginator:
withNextPageToken()
withPreviousPageToken()
getNextPageToken()
getPreviousPageToken()

why? common to all paginators:

  • allow to set or get next page
  • allow to set or get previous page

Paginators can be divided into two main groups:

  • navigate to any page
  • navigate only to the previous and next pages. This functionalty supported by "navigate to any page".

This comment has been minimized.

Copy link
@roxblnfk

roxblnfk Feb 4, 2020

Author Member
  • allow to set or get next page
  • allow to set or get previous page

But OffsetPaginator does not operate with tokens.
He has enough functions getNextPage(): int, getPreviousPage(): int, withNextPage() and withPreviousPage() without a Tokens and arguments
Current withNextPage() and withPreviousPage() are aliases of withCurrentPage() method with string->int casts

So i think we can create also KeysetPaginatorInterface with extended methods like withPreviousPage($token)

This comment has been minimized.

Copy link
@samdark

samdark Feb 4, 2020

Member

But OffsetPaginator does not operate with tokens.

It does. Both tokens accept page number.

This comment has been minimized.

Copy link
@kamarton

kamarton Feb 5, 2020

Contributor

@kamarton any concrete use-cases?

The current implementation is used a offsetable data reader (offset + limit). What about data readers based on page size and page number, e.g. APIs.

src/Paginator/PaginatorException.php Outdated Show resolved Hide resolved
tests/Paginator/OffsetPaginatorTest.php Show resolved Hide resolved
tests/Paginator/OffsetPaginatorTest.php Outdated Show resolved Hide resolved
src/Paginator/OffsetPaginator.php Outdated Show resolved Hide resolved
roxblnfk added 3 commits Feb 3, 2020
roxblnfk added 2 commits Feb 4, 2020
@roxblnfk roxblnfk mentioned this pull request Feb 4, 2020
4 of 4 tasks complete
@xepozz

This comment has been minimized.

Copy link
Contributor

xepozz commented Feb 4, 2020

@roxblnfk why did you add cache back?
It looks like it doesn't cache anything. What's the secret?

@roxblnfk

This comment has been minimized.

Copy link
Member Author

roxblnfk commented Feb 4, 2020

@roxblnfk why did you add cache back?
It looks like it doesn't cache anything. What's the secret?

Only the instance of DataReader is cached when we call the read() function
This allows us to successfully use the cache in the DataReader

@xepozz

This comment has been minimized.

Copy link
Contributor

xepozz commented Feb 5, 2020

But we can use the cache without it directly in readers.

@roxblnfk

This comment has been minimized.

Copy link
Member Author

roxblnfk commented Feb 5, 2020

But we can use the cache without it directly in readers.

This way is very difficult. Because for each call of read() method the Paginator will generate new DataReader object using immutable methods

@kamarton

This comment has been minimized.

Copy link
Contributor

kamarton commented Feb 5, 2020

This allows us to successfully use the cache in the DataReader

The current implementation (DataReader Interface) is not supported caching mechanism. I prefer to use the pagination layer to caching for prevent repeated read from data reader.

@roxblnfk

This comment has been minimized.

Copy link
Member Author

roxblnfk commented Feb 5, 2020

The current implementation (DataReader Interface) is not supported caching mechanism.

The task of the paginator is to tell the DataReader what from position to read and how much. If the DataReader reads from an array, then caching in the Paginator will be redundant.
If you use the same DataReader in different Paginators, then the DataReader will be more successful in caching (for example, the total items count).
The DataReader knows better where he is reading from and whether to use caching.


If we will return caching in the Paginator, then here are some options I can offer:

1 Make an Interface for the DataReader who will mean that data caching is not necessary (either DataReader provides caching himself, or caching is just no need)
2 Make a second paginator with a cache inside. To do this, we will need to leave the OffsetPaginatorInterface (now it is planned to remove OffsetPaginatorInterface, because we have not convinced @samdark)

@xepozz

This comment has been minimized.

Copy link
Contributor

xepozz commented Feb 5, 2020

This way is very difficult. Because for each call of read() method the Paginator will generate new DataReader object using immutable methods

It can be simplified to the following algorith:

  1. Someone calls read()
  2. Paginator will check if the reader has been tuned with offsets/limits?
    2.1. If yes then the paginator will use already tuned reader
    2.2. If no then the paginator must adjust the reader and use it

Let's say now we have not the cachedReader, only have one dataReader. If it already configured then we need to return just $this->dataReader->read(). DataReader can check the cache and return already exists values.

By this I wanted to say that the paginator will not implement any caching mechanisms. It must be implemented in DataReaders.

Did I correctly express my thought?

@roxblnfk

This comment has been minimized.

Copy link
Member Author

roxblnfk commented Feb 5, 2020

It can be simplified to the following algorith:

  1. Someone calls read()
  2. Paginator will check if the reader has been tuned with offsets/limits?
    2.1. If yes then the paginator will use already tuned reader
    2.2. If no then the paginator must adjust the reader and use it

Let's say now we have not the cachedReader, only have one dataReader. If it already configured then we need to return just $this->dataReader->read(). DataReader can check the cache and return already exists values.

  • We cannot check if DataReader has been tuned. Yes, we can use some flag for this or we can do tuning DataReader in withPage() and withPageSize() methods. But:
  • If we overwrite the original DataReader (from the constructor) with the configured one, we will receive an erroneous value when requesting the total number of records (getTotalItems() method). For this to work fine, it will be necessary to cache the total count of items along with the DataReader and pass it to the next Paginator instance when using immutable methods

Anyway, we will have to cache something

@samdark
samdark approved these changes Feb 5, 2020
@samdark samdark merged commit 1dae24f into yiisoft:master Feb 5, 2020
3 checks passed
3 checks passed
Scrutinizer Analysis: 6 new issues, 49 updated code elements – Tests: passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/styleci/pr The analysis has passed
Details
@samdark

This comment has been minimized.

Copy link
Member

samdark commented Feb 5, 2020

Merged. Please raise caching / interface concerns separately. Thanks.

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

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.