-
Notifications
You must be signed in to change notification settings - Fork 259
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
RED-2002 Iterator Associations #518
Conversation
ecoologic
commented
Nov 8, 2023
- Allows lib consumers to define what method they want to call for their iterator
- Better docs on upgrading
* Change sorting with: `$params = ['sort' => '-updated_at'];` | ||
* Refer to the docs for details, including allowed sort fields | ||
* Combine everything: `$params = ['page[size]' => 2, 'sort' => 'updated_at', 'extra' => 'param'];` | ||
|
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.
Removing dup.
@@ -90,6 +90,7 @@ public static function send( | |||
if ($client->getAuth()) { | |||
list ($request, $requestOptions) = $client->getAuth()->prepareRequest($request, $requestOptions); | |||
} | |||
// echo "\nExternal API call: " . $request->getMethod() . " " . $request->getUri() . "\n"; |
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.
I'm keeping these commented code (above too) for now.
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.
Just a couple more comments
README.md
Outdated
} | ||
``` | ||
|
||
Where `resources` is the collection, eg: `$client->tickets()`. This can be useful for filter endpoints like [active automations](https://developer.zendesk.com/api-reference/ticketing/business-rules/automations/#list-active-automations). However, in this common case where you only need to change the method from `findAll()` to `findActive()` there's a better shortcut: |
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.
I'm finding this part to be slightly confusing. I'm assuming that this
Where
resources
is the collection, eg:$client->tickets()
.
Is referring to this code on line 167
$iterator = PaginationIterator($client->resources(), $strategy);
We might want to make it more clear that this is what we are referring to. Maybe we could move the code from line 167 to be above the code sample?
We could say something like:
In the code sample below, where we have resources()
as a placeholder for a collection like tickets
or users
So if you want to iterate through the tickets
collection, you would use the following line of code to retrieve the iterators for that collection:
$iterator = PaginationIterator($client->tickets(), $strategy);
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.
OK, I see your point. I will amend but trying to keep it short.
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.
Look good! Nice work!