Skip to content

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
Contributor

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