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

pagination should be easier #328

Merged
merged 2 commits into from
Mar 13, 2023
Merged

pagination should be easier #328

merged 2 commits into from
Mar 13, 2023

Conversation

gabrielsroka
Copy link
Contributor

@gabrielsroka gabrielsroka commented Nov 16, 2022

currently, pagination requires many lines of code:

async with Client() as client:
    users, resp, _ = await client.list_users()
    while users:
        for user in users:
            print(user.profile.login)
        users, _ = await resp.next() if resp.has_next() else (None, None)

my 2 line PR would reduce some of the boilerplate code (the last line above ^^^) to:

        users, _ = await resp.next()

it'd be even nicer to have an async generator to either return each page, or each object in the page (eg, Users). basically take the 5 lines above and move them into the SDK itself. then use async for to get the users:

# in the SDK `UserClient` class:
async def page_users(self):
    users, resp, _ = await self.list_users()
    while users:
        for user in users:
            yield user
        users, _ = await resp.next()

# call the SDK code:
async for user in client.page_users():
    print(user.profile.login)

this is more Pythonic, and similar to how other Okta SDKs work, eg, Node.js (https://github.com/okta/okta-sdk-nodejs#serial-or-parallel-synchronous-work), and I think Java, too, but I can't tell from the example.

Node.js example https://github.com/okta/okta-sdk-nodejs#list-all-org-users

// Node.js example

// You can also use async iterators.
for await (let user of client.listUsers()) {
    console.log(user);
}

my code doesn't handle errors, but it could. error handling should use try--it's more Pythonic.

see also #326 (comment)

@bretterer bretterer merged commit 6610e62 into okta:master Mar 13, 2023
@gabrielsroka
Copy link
Contributor Author

gabrielsroka commented Mar 15, 2023

it looks like this was rolled back due to breaking tests. is there a plan to fix the tests?

# if not self.has_next():
# return (None, None) #This causes errors with our async testing

@bretterer
Copy link
Collaborator

Hi there! @gabrielsroka, thank you for your initial PR. Unfortunately, at this time, it is not on our roadmap to re-enable after finding it broke our tests. However, I have created an internal ticket to track this as a feature to re-enable in the future. We appreciate your feedback and will keep it in mind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants