-
Notifications
You must be signed in to change notification settings - Fork 11
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
Events TypedDict typing #283
base: main
Are you sure you want to change the base?
Conversation
def construct_from_response(cls, response: dict): | ||
instance = cls() | ||
for k, v in response.items(): | ||
if k == "domains": |
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.
not sure if there's a better way than to explicitly add a check against the key so it doesn't all just become nested dicts
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.
In terms of a quick implementation, this will work.
However, this also feels like we are reinventing the wheel. It's object deserialization based on a schema. This is a solved problem and there are a number of libraries that would do this for us like Marshmallow ... and pydantic, typeguard, typical, or pytypes
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.
@jonatascastro12 or @PaulAsjes Do you have thoughts on ☝️ ?
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.
Yeah... I can see the limitations now. Nested objects are complicated and for having TypedDicts we need to repeat the attributes. I'm considering pydantic
which is widely used. Here's what I got:
from pydantic import BaseModel
from typing import TypedDict, List, Optional, Literal
from enum import Enum
class DirectoryType(Enum):
AZURE_SCIM_v2 = "azure scim v2.0"
BAMBOO_HR = "bamboohr"
BREATHE_HR = "breathe hr"
CEZANNE_HR = "cezanne hr"
CYBERARK_SCIM_v2 = "cyperark scim v2.0"
FOURTH_HR = "fourth hr"
GENERIC_SCIM_v2 = "generic scim v2.0"
GOOGLE = "gsuite directory"
HIBOB = "hibob"
JUMPCLOUD_SCIM_v2 = "jump cloud scim v2.0"
OKTA_SCIM_v2 = "okta scim v2.0"
ONELOGIN_SCIM_v2 = "onelogin scim v2.0"
PEOPLE_HR = "people hr"
PERSONIO = "personio"
PINGFEDERATE_SCIM_v2 = "pingfederate scim v2.0"
RIPPLING_SCIM_v2 = "rippling v2.0"
SFTP = "sftp"
SFTP_WORKDAY = "sftp workday"
WORKDAY = "workday"
class DirectoryState(Enum):
ACTIVE = "active"
DELETING = "deleting"
class DirectoryEventData(BaseModel):
id: str
name: str
type: DirectoryType
state: DirectoryState
organization_id: str
created_at: str
updated_at: str
object: Literal["directory"]
response = {
"id": "Test",
"name": "Test",
"type": "gsuite directory",
"state": "active",
"organization_id": "org_123",
"created_at": "2020-12-30T10:00:00Z",
"updated_at": "2020-12-30T10:00:00Z",
"object": "directory",
}
obj = DirectoryEventData.parse_obj(response)
print(obj)
directory = DirectoryEventData.parse_obj(obj)
print(directory)
another = DirectoryEventData(**directory.model_dump())
print(another)
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.
hmm in that the "state" is still just a string directory from the json reqponse, as opposed to the enum type (which would be DirectoryState.Active) no?
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.
@jonatascastro12 can you paste what the output of these lines is? (I think it'll directly answer ameesha's question)
obj = DirectoryEventData.parse_obj(response)
print(obj)
directory = DirectoryEventData.parse_obj(obj)
print(directory)
another = DirectoryEventData(**directory.model_dump())
print(another)
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.
The output. Those enums come typed as expected 👍 .
id='Test' name='Test' type=<DirectoryType.GOOGLE: 'gsuite directory'> state=<DirectoryState.ACTIVE: 'active'> domains=[OrganizationDomain(id='Test', domain='Test', object='organization_domain')] organization_id='org_123' created_at='2020-12-30T10:00:00Z' updated_at='2020-12-30T10:00:00Z' object='directory'
id='Test' name='Test' type=<DirectoryType.GOOGLE: 'gsuite directory'> state=<DirectoryState.ACTIVE: 'active'> domains=[OrganizationDomain(id='Test', domain='Test', object='organization_domain')] organization_id='org_123' created_at='2020-12-30T10:00:00Z' updated_at='2020-12-30T10:00:00Z' object='directory'
id='Test' name='Test' type=<DirectoryType.GOOGLE: 'gsuite directory'> state=<DirectoryState.ACTIVE: 'active'> domains=[OrganizationDomain(id='Test', domain='Test', object='organization_domain')] organization_id='org_123' created_at='2020-12-30T10:00:00Z' updated_at='2020-12-30T10:00:00Z' object='directory'
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.
Those are 3 equal objects. I just wanted to show ways that you can initialize, either with the same object, a dictionary or key arguments.
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.
Are the dsync.group.user_added
and dsync.group.user_removed
events coming in a follow PR?
Does this require changes to the WorkOS Docs?
Since we have not changed the API resource types, not yet. But when we do, then yes I think we will need to.
workos/event_objects/directory.py
Outdated
class OrganizationDomain(TypedDict): | ||
id: str | ||
domain: str | ||
|
||
|
||
class WorkOSOrganizationDomain: |
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.
Should these also have object: Literal["organization_domain"]
?
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.
ooh missed that, will add
workos/event_objects/directory.py
Outdated
class WorkOSDirectoryActivatedEvent: | ||
event: Literal["dsync.activated"] = "dsync.activated" | ||
id: str | ||
created_at: str | ||
data: WorkOSDirectoryEventWithLegacyFields | ||
|
||
@classmethod | ||
def construct_from_response(cls, response: dict): | ||
instance = cls() | ||
for k, v in response.items(): | ||
if k == "data": | ||
setattr( | ||
instance, | ||
k, | ||
WorkOSDirectoryEventWithLegacyFields.construct_from_response(v), | ||
) | ||
else: | ||
setattr(instance, k, v) | ||
|
||
return instance | ||
|
||
def to_dict(self) -> DirectoryActivatedEvent: | ||
return self.__dict__ | ||
|
||
def __str__(self): | ||
return f"{self.__class__.__name__} {self.to_dict()}" |
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.
Maybe this refactor can come later, but every single Event we define will have...
event: Literal[EVENT_NAME_LITERAL] = EVENT_NAME_LITERAL
id: str
created_at: str
data: EVENT_DATA_CLASS
Can we have a single WorkOSEvent
base class that's parameterized with the two bits of data that actually change? I think the actual implementation of the class looks the same across all events.
i hadn't done them all, just cause i wanted to make sure how i'm typing it is correct. let me switch this to a draft to make that clear. i'll add the membership stuff in a commit |
I'm totally cool with the membership stuff being part of this PR or a later one. It's on your radar. That's all that matters. |
Description
Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.