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

Add async support for Directory Sync #288

Conversation

mattgd
Copy link
Contributor

@mattgd mattgd commented Jul 24, 2024

Description

Add async support for DirectorySync.

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

@mattgd mattgd self-assigned this Jul 24, 2024
@mattgd mattgd changed the title Add async support for DirectorySync Add async support for Directory Sync Jul 24, 2024
Copy link

linear bot commented Jul 24, 2024

Comment on lines +228 to +231
SyncOrAsyncListResource = Union[
Awaitable[AsyncWorkOsListResource],
WorkOsListResource,
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is FUN. I wanted this to be:

SyncOrAsyncListResource = Union[
    Awaitable[AsyncWorkOsListResource[ListableResource, ListAndFilterParams]],
    WorkOsListResource[ListableResource, ListAndFilterParams],
]

but Unions cannot be used as generics. This means I had to make the Protocol return type SyncOrAsyncListResource for list methods, instead of something like SyncOrAsyncListResource[Directory, DirectoryFilterParams].

Copy link

Choose a reason for hiding this comment

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

I think to do something similar for events I'm creating a union of all event types and using that as a bounds= constraint on a type variable.... I don't know if that's helpful at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me give that a go.

@mattgd mattgd requested a review from tribble July 24, 2024 19:14
@mattgd mattgd marked this pull request as ready for review July 24, 2024 19:14
@mattgd mattgd requested a review from a team as a code owner July 24, 2024 19:14
@mattgd mattgd requested review from awolfden and removed request for a team July 24, 2024 19:14
Comment on lines +350 to +373
list_params = {
"limit": limit,
"before": before,
"after": after,
"order": order,
}

if group is not None:
list_params["group"] = group
if directory is not None:
list_params["directory"] = directory

response = await self._http_client.request(
"directory_users",
method=REQUEST_METHOD_GET,
params=list_params,
token=workos.api_key,
)

return AsyncWorkOsListResource(
list_method=self.list_users,
list_args=list_params,
**ListPage[DirectoryUser](**response).model_dump(),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR lacks deduplication of code like this. Might be a good thing for me to take a stab at either before or after I work on connections.

@@ -32,8 +33,8 @@ class PreparedRequest(TypedDict):
method: str
url: str
headers: httpx.Headers
params: NotRequired[Union[Dict, None]]
json: NotRequired[Union[Dict, None]]
params: NotRequired[Union[Mapping, None]]
Copy link

Choose a reason for hiding this comment

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

Oh. I just did the same thing in my PR. That's fine, git diffing should be good about any conflicts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I figured it should be okay. Just wanted to get my tests passing.

Copy link

@tribble tribble left a comment

Choose a reason for hiding this comment

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

Applause

@mattgd mattgd merged commit 79a5de3 into beta-5.0 Jul 24, 2024
4 checks passed
@mattgd mattgd deleted the feature/dsync-1725-add-async-operation-support-for-dsync-api-methods branch July 24, 2024 20:57
tribble pushed a commit that referenced this pull request Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants