Skip to content

Improve PaginatorInterface and ReadableDataInterface#150

Merged
vjik merged 15 commits intomasterfrom
improve
Nov 21, 2023
Merged

Improve PaginatorInterface and ReadableDataInterface#150
vjik merged 15 commits intomasterfrom
improve

Conversation

@vjik
Copy link
Member

@vjik vjik commented Nov 16, 2023

Q A
Is bugfix?
New feature? ✔️
Breaks BC? ✔️
Fixed issues -
  • Extract withLimit() from ReadableDataInterface to LimitableDataInterface.
  • PaginatorInterface extends ReadableDataInterface

@codecov
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f30a971) 100.00% compared to head (acb18a4) 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##              master      #150   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       249       254    +5     
===========================================
  Files             35        35           
  Lines            615       634   +19     
===========================================
+ Hits             615       634   +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@what-the-diff
Copy link

what-the-diff bot commented Nov 16, 2023

PR Summary

This Pull Request focuses on improving the functionality and efficiency of both the KeysetPaginator.php and OffsetPaginator.php classes along with updating their dependencies.

  • Updated Psalm Configuration Files
    The psalm-php80.xml and psalm.xml configuration files have been updated to better find unused data entries and to disable the detection of unused code.

  • Improved Paginator System
    Both KeysetPaginator.php and OffsetPaginator.php classes are updated to use LimitableDataInterface which brings new validation and exception handling mechanisms. A new method readOne() has been added to further facilitate data reading.

  • PaginatorInterface Enhancement
    The PaginatorInterface.php now extends ReadableDataInterface, enhancing its capabilities and bringing it up-to-date with recent technologies.

  • Introduced LimitableDataInterface
    A new interface, LimitableDataInterface.php, has been introduced, which brings more refinement to how the system handles data reading limitations.

  • ReadableDataInterface Update
    The withLimit method from ReadableDataInterface.php has been removed, which suggests changes to how data reading limits are handled in the future.

  • DataReaderInterface Dependency Update
    The DataReaderInterface.php now uses LimitableDataInterface, reinforcing the system's ability to handle data reading limitations.

@vjik vjik marked this pull request as ready for review November 16, 2023 10:38
@vjik vjik added the status:code review The pull request needs review. label Nov 16, 2023
@vjik vjik requested review from a team and terabytesoftw November 16, 2023 10:38
@roxblnfk
Copy link
Member

Too many changes for one PR. You should consider breaking it down into several

Copy link
Member

@roxblnfk roxblnfk left a comment

Choose a reason for hiding this comment

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

I'm not sure that BC is necessary for this package

* @extends ReadableDataInterface<TKey, TValue>
*/
interface PaginatorInterface
interface PaginatorInterface extends ReadableDataInterface
Copy link
Member

@roxblnfk roxblnfk Nov 16, 2023

Choose a reason for hiding this comment

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

Are you sure that PaginatorInterface::read() and ReaderInterface::read() are compatible in the meaning?

    /**
     * Get iterable for the data set.
     *
     * @return iterable Iterable for the data.
     * @psalm-return iterable<TKey, TValue>
     */
    public function read(): iterable;
    /**
     * Get iterator that could be used to read currently active page items.
     *
     * @throws PaginatorException If page specified is not found.
     *
     * @return iterable Iterator with items for the current page.
     * @psalm-return iterable<TKey, TValue>
     */
    public function read(): iterable;

Copy link
Member

Choose a reason for hiding this comment

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

By the way, this was the main reason why the Paginator didn't implement the DataReaderInterface from the very beginning.
This could lead to confusion and unexpected behavior. One user might expect that passing a Paginator would read all the data, while another user might expect it to read only one page.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand reason. You talk about implementation details. For users of ReadableDataInterface it doesn't matter.

Co-authored-by: Aleksei Gagarin <roxblnfk@ya.ru>
@vjik
Copy link
Member Author

vjik commented Nov 16, 2023

Too many changes for one PR. You should consider breaking it down into several

This is related changes that don't make sense individually.

I'm not sure that BC is necessary for this package

These changes cannot be made without breack BC.

vjik and others added 3 commits November 17, 2023 12:49
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
@vjik vjik merged commit e2fd7e0 into master Nov 21, 2023
@vjik vjik deleted the improve branch November 21, 2023 08:06
@roxblnfk roxblnfk added this to the 2.0.0 milestone Nov 21, 2023
/**
* Data that could be limited.
*/
interface LimitableDataInterface
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add "extends ReadableInterface" there

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

Labels

status:code review The pull request needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants