Preserve the pagination data in the result set #43
Conversation
|
Looks okay (besides coding standards errors), I'll just consult someone to ensure this is the right way. |
Codecov Report
@@ Coverage Diff @@
## master #43 +/- ##
============================================
+ Coverage 71.57% 73.01% +1.43%
- Complexity 352 360 +8
============================================
Files 13 13
Lines 876 893 +17
============================================
+ Hits 627 652 +25
+ Misses 249 241 -8
Continue to review full report at Codecov.
|
src/ResultSet.php
Outdated
| * @param array $pagination Array of pagination data | ||
| * @param int|null $total The total amount of resources available | ||
| * @param array $total Array of pagination data | ||
| * @param null $pagination The total amount of resources available |
There was a problem hiding this comment.
You have mixed up the two params.
|
So adding paging data to result is fine but I would prefer some changes:
|
|
I have problems with these requested changes.
|
Pagination is a pretty standard thing. Each provider may give that information in a different way using different names but at the end it's the same. For e.g. every key shown under The whole point of this plugin is to provided a consistent representation of different webservices/APIs. That will involve some overheard. If you don't like that than perhaps this is not the right tool for you.
Yes allowing different types for an argument is not ideal but this is just temporary to maintain BC. In next major release it would only take an array. This is still better than adding an unnecessary extra argument when total itself is a paging param.
Knowingly using names for new methods which could change in future is silly. I doubt currently anyone is willing to spend the time updating all methods. So let's just use proper name for new method to be added. |
|
If the pagination is standard like this, then we should consider using a value object, which implements an interface and typehint against that in the method. This way developers can assign their own values to that object, and the interface will ensure consistency. Using a value object would also solve the params problem and avoid the mixed method requirement. As well as making any transition to PHP 7.0 much easier. I value consistency above all else. The api of this plugin is already a mess, as detailed in #40 and #36. Using methods which match the way the plugin currently works means that it will be easier to guess the method. I'd be happy to go through and update all the method names if it's required. All in all this discussion is making me think it's worth implementing and releasing a new major version with these changes. |
You might be right, perhaps it's worth me maintaining my own fork. |
|
I'll have to revisit this, as the plugin does need some pagination support. |
Resolves #42
When reading apis which are paginated, it's helpful to have a recursive function. However it needs an exit condition, so preserving the pagination data it returns allows for this by keeping the pagination data with the result set.