Type Hint Responses [WIP]

[guacs]

This is WIP PR for #199.

@ramnes, I just wanted to confirm whether this kind of an approach is fine or not. Also, I was thinking that instead of having to maintain a separate async and synchronous versions of the endpoints, we could use something like unasyncd.

Once this is confirmed, I'll go ahead and get to typing all the responses by comparing with the official docs and the official Javascript client.

[ramnes]

Hey @guacs! Thanks a lot for this first iteration.

Quick thoughts:

• I'd prefer to avoid code duplication, and I really want to avoid manual code duplication at all costs; if we don't find a simpler way, automated code duplication may be okay but I'd like to see a PoC first.
• I want us to stay as close as possible to the JS implementation, so we should:
  1. replicate the types defined here
  2. expose them from api_endpoints (if we don't want to have a single big file, we could write a private _types.py module that we import in api_endpoints, and add add to its __all__)

Can't guarantee anything but I may try to work on a PoC as well. :)

In any case, please feel free to modify the code directly, that would actually help me to understand your ideas by providing me a diff to read!

[guacs]

Yeah I definitely agree that the code duplication would definitely be a burden. I'll create a PoC for just the UsersEndpoint (this has the simplest types that would need to be implemented) with both an async version and a sync version created by unasyncd.

On the point of the types, I'll try my best to keep it in sync with the one that is in the typescript client, but there might be some changes that will need to be made to accommodate for the differences in the type system.

[guacs]

@ramnes I have the basic setup with just the user responses typed.

The _sync_api_endpoints was created using the unasyncd library I mentioned before. However, I did have to manually go and change the type of the client in the base endpoint class as well as rename it from AsyncEndpoint to Endpoint manually. The rest of it was automatically created with the following command: unasyncd notion_client/_async_api_endpoints.py:notion_client/_sync_api_endpoints.py

I ran the tests and all of them passed except for one test in helpers with the following error:

tests/test_helpers.py::test_is_full_page_or_database - notion_client.errors.APIResponseError: API token is invalid.

[guacs]

Also, regarding the types I had some doubts:

• How closely do you want to follow the official TS types? Looking through it there is a lot of room for reusing already described types which are redefined in the TS types since that one is autogenerated.
• Do you prefer to keep the naming the same as that in the TS types or switch to more user-friendly type names? For example, the type of the response for the retrieve user endpoint is UserObjectResponse which could be simplified to User if we want to. Personally, I'm fine with either approach.

Also, please note that we will have to create some new types like PersonDetails due to the differences in how the types can be described in the two languages.

[ramnes]

Looks better, thanks! I'm still quite reluctant to duplicate the whole endpoints file to handle types. If I understand correctly, the duplication is only due to "the SyncAsync[T] [that] does not work well with type checkers"? What doesn't work exactly?

[guacs]

So let's assume the UsersEndpoint.retrieve has a return type of SyncAsync[User] with User being dict[str, Any].

Mypy complains with the following: Incompatible types in "await" (actual type "dict[str, Any] | Awaitable[dict[str, Any]]", expected type "Awaitable[Any]") [misc]

Pyright also complains with the following: 'User' is not awaitable (reportGeneralTypeIssues)

[guacs]

Btw, mypy and pyright issues are for the following code:

from notion_client.client import AsyncClient

async def main():

    async_client = AsyncClient()

    user = await async_client.users.retrieve("user-id")

[ramnes]

What about something like this? Mypy seems happy:

 class UsersEndpoint(Endpoint):
-    def list(self, **kwargs: Any) -> SyncAsync[Any]:
+    @overload
+    async def list(self, **kwargs: Any) -> List[UserObjectResponse]:
+        ...
+
+    def list(self, **kwargs: Any) -> List[UserObjectResponse]:
         """Return a paginated list of [Users](https://developers.notion.com/reference/user) for the workspace.

         *[🔗 Endpoint documentation](https://developers.notion.com/reference/get-users)*

Or something like this, if we don't want to lie to type checkers:

 class UsersEndpoint(Endpoint):
-    def list(self, **kwargs: Any) -> SyncAsync[Any]:
+    @overload
+    def list(self, **kwargs: Any) -> Awaitable[List[UserObjectResponse]]:
+        ...
+
+    def list(self, **kwargs: Any) -> List[UserObjectResponse]:
         """Return a paginated list of [Users](https://developers.notion.com/reference/user) for the workspace.

[guacs]

Mypy is not complaining about the code the user would write, but it is raising an error for the implementation within the library though. Do you have just one overload? I'm getting the following errors when running mypy on the api_endpoints.py file:

• error: Single overload definition, multiple required [misc]
• Overloaded function implementation cannot produce return type of signature 1 [misc]

[ramnes]

Yeah right, this is not the proper way to use @overload, it needs at least two alternatives and it can't use the return type to decide which alternative to use... TIL. :)

After a bit more digging, here is what I ended up with: https://github.com/ramnes/notion-sdk-py/compare/ramnes:1a8bdd4...ramnes:fe90378

With such a users.py file:

import asyncio
import os
from pprint import pprint

from typing import TYPE_CHECKING

from notion_client import AsyncClient, Client

def main_sync() -> None:
    notion = Client(auth=os.environ["NOTION_TOKEN"])
    users = notion.users.list()

    if TYPE_CHECKING:
        reveal_type(notion.users.parent)
        reveal_type(users)

    pprint(users)

async def main_async() -> None:
    notion = AsyncClient(auth=os.environ["NOTION_TOKEN"])
    users = await notion.users.list()

    if TYPE_CHECKING:
        reveal_type(notion.users.parent)
        reveal_type(users)

    pprint(users)

main_sync()
asyncio.run(main_async())

It correctly reveals the types:

$ mypy users.py
users.py:15: note: Revealed type is "notion_client.client.Client"
users.py:16: note: Revealed type is "builtins.list[builtins.dict[builtins.str, Any]]"
users.py:26: note: Revealed type is "notion_client.client.AsyncClient"
users.py:27: note: Revealed type is "builtins.list[builtins.dict[builtins.str, Any]]"
Success: no issues found in 1 source file

Thoughts?

Everything is in the poc-types branch if you want to play with it.

[guacs]

This works perfectly! Yeah this is a far better approach. I think we can just go with this instead of the different implementations for sync and async.

[guacs]

Also, regarding the types I had some doubts:

• How closely do you want to follow the official TS types? Looking through it there is a lot of room for reusing already described types which are redefined in the TS types since that one is autogenerated.
• Do you prefer to keep the naming the same as that in the TS types or switch to more user-friendly type names? For example, the type of the response for the retrieve user endpoint is UserObjectResponse which could be simplified to User if we want to. Personally, I'm fine with either approach.

Also, please note that we will have to create some new types like PersonDetails due to the differences in how the types can be described in the two languages.

Now I think we just need to make a decision regarding these points, and we can progress with the changes.

[ramnes]

Cool, happy that you like it!

Just a quick note: I think we should use __future__.annotations to make a lot of the typing stuff simpler. It looks like it has been ported back to 3.7, which is great because ideally that's the first version I would like to support. If for some reason anything related to deferred annotations doesn't work in a particular version of Python, please tell me and we'll consider dropping support for it.

Regarding the types, I'd prefer that we stay consistent with the TypeScript SDK names. Thanks!

[ramnes]

The PR is stale so I'll close it, but feel free to open a new PR if you feel like it!