Refactor and streamline organization and set base for SDK typing #284

jonatascastro12 · 2024-07-12T23:18:08Z

Create new WorkOsListResource to fix .auto_pagination_iter() for list methods
Add pydandic for setting new resource classes for Organization and other entities (come later)
Add types-requests as mypy complained for typing requests lib
Add mypy (initially scoping to workos/organizations.py)
Refactored workos/organizations.py - methods return pydandic models
- removed .list_organizations_v2() method since they were meant to work with .auto_pagination_iter(), but it wasn't functional
- create/update methods are consistent with using args
- typed all methods arguments
Removed deprecated fields on organizations

Removed deprecated organization methods and updated organization tests. Also, migrated organization.py to Pydantic models, and added mypy typing for more strict type checking. Additionally, enhance auto-paging to work with new models and restructured fixtures for organization tests.

tribble · 2024-07-12T23:50:09Z

workos/organizations.py

-        order=None,
-    ):
+        domains: Optional[List[str]] = None,
+        limit: Optional[int] = None,


If limit has a default value of RESPONSE_LIMIT below, any reason not to make that the default value here?

Suggested change

limit: Optional[int] = None,

limit: Optional[int] = RESPONSE_LIMIT,

... and remove the default_limit = True below. It doesn't seem to do anything.

…oughts on the approach

…s to a typing directory

tribble · 2024-07-22T17:27:39Z

tests/conftest.py

@@ -45,7 +47,7 @@ def inner(method, response_dict, status_code, headers=None):
        def mock(*args, **kwargs):
            return MockResponse(response_dict, status_code, headers=headers)

-        monkeypatch.setattr(requests, method, mock)
+        monkeypatch.setattr(requests, "request", mock)


I changed how we call requests, rather than using get_attr to retrieve the function by name, always wall request.request and pass in the method

tribble · 2024-07-22T17:30:12Z

tests/conftest.py

+
+
+@pytest.fixture
+def mock_pagination_request(monkeypatch):


I don't like how complex this test function is, but we need to roughly emulate the behavior of a pagination endpoint and return different pages of data in successive calls.

tribble · 2024-07-22T17:30:38Z

tests/test_directory_sync.py

@@ -18,110 +22,19 @@ def mock_users(self):
        return {
            "data": user_list,
            "list_metadata": {"before": None, "after": None},
-            "metadata": {


All of this no longer used/needed

tribble · 2024-07-22T17:47:55Z

workos/directory_sync.py

+        limit: int = RESPONSE_LIMIT,
+        before: Optional[str] = None,
+        after: Optional[str] = None,
+        order: PaginationOrder = "desc",


Optional[PaginationOrder] = "desc" is not what we want here. That allows None as a possible value. This disallows None and does not require order to be specified.

tribble · 2024-07-22T17:48:25Z

workos/directory_sync.py

-            token=workos.api_key,
+        return WorkOsListResource(
+            list_method=self.list_users,
+            # TODO: Should we even bother with this validation?


My current opinion is no. But want to constrain scope of this PR and fix later

Loaded up the page before you posted this, but down to omit. Better to be consistent everywhere IMO, and I think on Friday we decided against explicit input validation like this.

👍 Will remove in a follow-on PR

tribble · 2024-07-22T17:50:04Z

workos/directory_sync.py

        """Delete one existing Directory.

        Args:
            directory (str): The ID of the directory to be deleted. (Required)

        Returns:
-            dict: Directories response from WorkOS.
+            None


Yeah. Our doc here was a lie. We return an empty 202

tribble · 2024-07-22T17:51:31Z

workos/typing/enums.py

+
+EnumType = TypeVar("EnumType", bound=Enum)
+
+# Identical to the literal or strings path, except meant for a literal of enum values.


Should I just delete this from the PR? Not being used at the moment.

Yep, can delete.

mattgd · 2024-07-22T17:44:53Z

workos/organizations.py


        params = {
            "domains": domains,
            "limit": limit,
            "before": before,
            "after": after,
-            "order": order or "desc",
+            "order": order,


With order previously being passed as None, this doesn't change the default ordering at all, right?

Previously if order was not passed in, the value would fall back to "desc". Instead that default moves to the function signature: order: PaginationOrder = "desc"

mattgd · 2024-07-22T17:47:11Z

workos/resources/directory_sync.py

+    # Toying around with the differences in type hinting or deserialization for enums vs literals. In the end pretty equivalent,
+    # it's a question of whether we want to present strings or DirectoryState.ACTIVE to the user
+    # leaving both options here for now until we settle on a preference


Should this comment be removed now?

mattgd · 2024-07-22T17:48:13Z

workos/resources/organizations.py

+class OrganizationDomain(WorkOSModel):
+    id: str
+    organization_id: str
+    object: Literal["organization"]


Should this be Literal["organization_domain"]?

It sure should

mattgd · 2024-07-22T17:52:43Z

workos/resources/list.py

+    # These fields end up exposed in the types. Does we care?
+    list_method: Callable = Field(exclude=True)
+    list_args: ListArgs = Field(exclude=True)


Where do these fields this show up in types exactly?

They would appear a developers IDE, like this

mattgd · 2024-07-22T17:53:21Z

workos/resources/list.py

+# add all possible generics of List Resource
+T = TypeVar("T", Organization, Directory, DirectoryGroup, DirectoryUser)


I feel like we could name this something better like "ListableResource".

mattgd · 2024-07-22T17:55:10Z

workos/organizations.py

+            # TODO: Should we even bother with this validation?
+            list_args=ListArgs.model_validate(params),


I feel like the call made Friday was that we wouldn't focus on inbound param validation. I'm okay with either, but we should probably be consistent.

Will remove in a follow-up.

* Refactor and streamline organization tests and resources Removed deprecated organization methods and updated organization tests. Also, migrated organization.py to Pydantic models, and added mypy typing for more strict type checking. Additionally, enhance auto-paging to work with new models and restructured fixtures for organization tests. * Switch directories over to new list resources and add typing * Mark version as alpha * Formatting and some type fixes * Add defaults for all optional fields * Type dsync getters * Possible path forward for unknown literal values * Move comments * Export the untyped type and a type guard. Leave a comment about my thoughts on the approach * Test out literals of strings vs enums and factor out types and helpers to a typing directory * Settled on an approach for untyped literals * A little refactoring * More accurate validation for UntypedLiteral * Comments reminding us to clean up * Add typing to the rest of the dsync methods * Just some formattin' * A little more cleanup to unify organizations and dsync * Fix organizations test * DirectorySync tests passing, but still need some cleanup * Do not bump version yet * Formatting * Clean up by removing the enum paths * Small fixed for compat * More formatting and compat * Start fixing dsync tests * Fix another dsync test * Fix more dsync tests * Fix params, optional allows None, instead can force default * Directory sync tests are green * Fix audit logs test * Fix organizations test * Fix auto paging iter tests * Remove debugging print * Fix user management tests * Switch override to typing_extensions * Build fixes. Proper dict override and remove duplicate model key * Add directory sync resource to mypy.ini * Pantera can't spell * Fix type and delete unused fixtures * Remove unused import * Update dsync tests to new pagination helper * Delete unused fixtures * Remove some unneeded comments * PR feedback * Remove trailing comma in mypy.ini --------- Co-authored-by: Peter Arzhintar <[email protected]>

jonatascastro12 requested a review from a team as a code owner July 12, 2024 23:18

jonatascastro12 requested a review from mthadley July 12, 2024 23:18

jonatascastro12 changed the title ~~Refactor and streamline organization tests and resources~~ Refactor and streamline organization and set base for SDK typing Jul 12, 2024

jonatascastro12 force-pushed the refactor-organization branch from b6fc19d to e9a1274 Compare July 12, 2024 23:20

tribble reviewed Jul 12, 2024

View reviewed changes

tribble added 19 commits July 15, 2024 17:58

Switch directories over to new list resources and add typing

5b8cc88

Mark version as alpha

4a874f8

Formatting and some type fixes

3de3412

Add defaults for all optional fields

9919273

Type dsync getters

77b411d

Possible path forward for unknown literal values

7665174

Move comments

fbc6d97

Export the untyped type and a type guard. Leave a comment about my th…

74be494

…oughts on the approach

Test out literals of strings vs enums and factor out types and helper…

24a64fe

…s to a typing directory

Settled on an approach for untyped literals

8369952

A little refactoring

abb9d66

More accurate validation for UntypedLiteral

a4945e6

Comments reminding us to clean up

0c55925

Add typing to the rest of the dsync methods

e8bc58a

Just some formattin'

37d2503

A little more cleanup to unify organizations and dsync

456b213

Fix organizations test

15da323

DirectorySync tests passing, but still need some cleanup

a39910c

Do not bump version yet

d05e4cb

tribble requested a review from mattgd July 19, 2024 16:28

tribble added 4 commits July 19, 2024 10:23

Formatting

5f6cbd0

Clean up by removing the enum paths

3c6f436

Small fixed for compat

46692f1

More formatting and compat

668ff6f

tribble added 10 commits July 19, 2024 15:16

Fix params, optional allows None, instead can force default

a56e276

Directory sync tests are green

3c1fc83

Fix audit logs test

e01930d

Fix organizations test

30a309a

Fix auto paging iter tests

f5d8efd

Remove debugging print

5e2ce34

Fix user management tests

96826e3

Switch override to typing_extensions

066324f

Build fixes. Proper dict override and remove duplicate model key

b64f786

Add directory sync resource to mypy.ini

de81bca

tribble reviewed Jul 22, 2024

View reviewed changes

Pantera can't spell

cc19195

tribble reviewed Jul 22, 2024

View reviewed changes

tribble added 4 commits July 22, 2024 10:32

Fix type and delete unused fixtures

04e89db

Remove unused import

834d28a

Update dsync tests to new pagination helper

b0fca23

Delete unused fixtures

b5e2711

tribble reviewed Jul 22, 2024

View reviewed changes

mattgd reviewed Jul 22, 2024

View reviewed changes

tribble added 3 commits July 22, 2024 13:23

Remove some unneeded comments

6da108d

PR feedback

2ee8b5d

Remove trailing comma in mypy.ini

737f2d7

tribble changed the base branch from main to beta-5.0 July 22, 2024 21:43

tribble merged commit 6810556 into beta-5.0 Jul 22, 2024
4 checks passed

tribble deleted the refactor-organization branch July 22, 2024 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and streamline organization and set base for SDK typing #284

Refactor and streamline organization and set base for SDK typing #284

jonatascastro12 commented Jul 12, 2024 •

edited

Loading

tribble Jul 12, 2024

tribble Jul 12, 2024

tribble Jul 22, 2024

tribble Jul 22, 2024

tribble Jul 22, 2024

tribble Jul 22, 2024

tribble Jul 22, 2024

mattgd Jul 22, 2024

tribble Jul 22, 2024

tribble Jul 22, 2024

tribble Jul 22, 2024

mattgd Jul 22, 2024

mattgd Jul 22, 2024

tribble Jul 22, 2024

mattgd Jul 22, 2024

mattgd Jul 22, 2024

tribble Jul 22, 2024

mattgd Jul 22, 2024

tribble Jul 22, 2024 •

edited

Loading

mattgd Jul 22, 2024

mattgd Jul 22, 2024

tribble Jul 22, 2024

	limit: Optional[int] = None,
	limit: Optional[int] = RESPONSE_LIMIT,


		EnumType = TypeVar("EnumType", bound=Enum)

		# Identical to the literal or strings path, except meant for a literal of enum values.

		# add all possible generics of List Resource
		T = TypeVar("T", Organization, Directory, DirectoryGroup, DirectoryUser)

		# TODO: Should we even bother with this validation?
		list_args=ListArgs.model_validate(params),

Refactor and streamline organization and set base for SDK typing #284

Refactor and streamline organization and set base for SDK typing #284

Conversation

jonatascastro12 commented Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribble Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatascastro12 commented Jul 12, 2024 •

edited

Loading

tribble Jul 22, 2024 •

edited

Loading