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

Type all dsync events #292

Merged
merged 8 commits into from
Jul 25, 2024
Merged

Type all dsync events #292

merged 8 commits into from
Jul 25, 2024

Conversation

tribble
Copy link

@tribble tribble commented Jul 25, 2024

Description

Add types for the rest of the Dsync events. Needed to refactor the API return type. The Event does not contain groups.

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.

@tribble tribble requested a review from mattgd July 25, 2024 21:39
@tribble tribble requested a review from a team as a code owner July 25, 2024 21:39
@@ -19,31 +19,41 @@ class BaseClient(Protocol):
_http_client: BaseHTTPClient

@property
def audit_logs(self) -> AuditLogsModule: ...
def audit_logs(self) -> AuditLogsModule:
Copy link
Author

Choose a reason for hiding this comment

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

... do we have different configs for black? Or different versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated the version of black in the repo. I believe if you try reinstalling the version matching the setup.py you, me and CI will be in sync.

Copy link
Author

Choose a reason for hiding this comment

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

Yup, needed to pip install -U black

Comment on lines +15 to +16
class Role(WorkOSModel):
slug: str
Copy link
Contributor

Choose a reason for hiding this comment

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

We could reuse this for org. memberships, so I may end up moving it.

Copy link
Author

Choose a reason for hiding this comment

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

Yup, I figured it might move around

class DirectoryGroupCreatedEvent(
EventModel[Literal["dsync.group.created"], DirectoryGroup]
):
event: Literal["dsync.group.created"]
Copy link
Author

Choose a reason for hiding this comment

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

When I set pylance to standard type checking rather than basic, it doesn't like this property override because it doesn't match the type on EventModel.

I'm going to ignore it for now, but it may rear its head again when I test what happens when an unrecognized event type gets parsed.

@tribble tribble merged commit 47cd107 into beta-5.0 Jul 25, 2024
5 checks passed
@tribble tribble deleted the type-all-dsync-events branch July 25, 2024 21:54
tribble added a commit that referenced this pull request Jul 30, 2024
* Add types for directory group events

* Add directory user events

* Under refactor of separate type files, we can do that later across all resources if we want

* Add group membership events

* Fix types for user events, they do not contain groups

* Reformat

* Reformat... again

* mypy fixes
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