yanyongyu / githubkit

The modern, all-batteries-included GitHub SDK for Python, including rest api, graphql, webhooks, like octokit!
MIT License
159 stars 21 forks source link

Type checking error when using github.paginate #27

Open netomi opened 1 year ago

netomi commented 1 year ago

When trying to use the paginate function like that:

org_id = "blabla"
github = GitHub("token")
for repo in github.paginate(github.rest.repos.list_for_org, org=org_id):
    print(repo)

I get the following type checking error in PyCharm 2023.1:

Unexpected type(s): ((org: str, type: Literal["all", "public", "private", "forks", "sources", "member"] | Unset, sort: Literal["created", "updated", "pushed", "full_name"] | Unset, direction: Literal["asc", "desc"] | Unset, per_page: Unset | int, page: Unset | int) -> Response[list[MinimalRepository]]) Possible type(s): ((org: str, type: Literal["all", "public", "private", "forks", "sources", "member"] | Unset, sort: Literal["created", "updated", "pushed", "full_name"] | Unset, direction: Literal["asc", "desc"] | Unset, per_page: Unset | int, page: Unset | int) -> Response | (org: str, type: Literal["all", "public", "private", "forks", "sources", "member"] | Unset, sort: Literal["created", "updated", "pushed", "full_name"] | Unset, direction: Literal["asc", "desc"] | Unset, per_page: Unset | int, page: Unset | int) -> Awaitable[Response]) ((org: str, type: Literal["all", "public", "private", "forks", "sources", "member"] | Unset, sort: Literal["created", "updated", "pushed", "full_name"] | Unset, direction: Literal["asc", "desc"] | Unset, per_page: Unset | int, page: Unset | int) -> Response | (org: str, type: Literal["all", "public", "private", "forks", "sources", "member"] | Unset, sort: Literal["created", "updated", "pushed", "full_name"] | Unset, direction: Literal["asc", "desc"] | Unset, per_page: Unset | int, page: Unset | int) -> Awaitable[Response])

running it works though.

I could fix that error, changing the following code (there are 3 overriden paginate methods, I did change all of them accordingly):

    @staticmethod
    def paginate(
        request: R[CP, CT],
        page: int = 1,
        per_page: int = 100,
        map_func: Optional[Callable[[Response[CT]], List[RT]]] = None,
        *args: CP.args,
        **kwargs: CP.kwargs,
    ) -> Paginator[RT]:
        return Paginator(request, page, per_page, map_func, *args, **kwargs)  # type: ignore

to

    @staticmethod
    def paginate(
        request: R,
        page: int = 1,
        per_page: int = 100,
        map_func: Optional[Callable[[Response[CT]], List[RT]]] = None,
        *args: CP.args,
        **kwargs: CP.kwargs,
    ) -> Paginator[RT]:
        return Paginator(request, page, per_page, map_func, *args, **kwargs)  # type: ignore

so not providing type arguments to R itself, which is not clear to me why this even works as R is not a generic type imho.

yanyongyu commented 1 year ago

which type checker you are using? the typevar in your change is used to restrict the map_func and extra params. I'm using VSCode and the static type check is OK with Pylance/Pyright

yanyongyu commented 1 year ago

paginator func has overload types: https://github.com/yanyongyu/githubkit/blob/42de20d2012e95f0040909e61bcee3c1f1c72551/githubkit/github.py#L153-L176

the type annotations in the implement should be ignored by checker

The @overload-decorated definitions are for the benefit of the type checker only, since they will be overwritten by the non-@overload-decorated definition, while the latter is used at runtime but should be ignored by a type checker.

netomi commented 1 year ago

I am using the builtin type check of PyCharm. The IDE highlights the errors. Its maybe a bug in the checker itself. It appeared to me that you would like to use the TypeVar from the function parameter for other uses. I am not an expert with python type annotations so maybe that is correct to do so.

yanyongyu commented 1 year ago

Can you test the checker with following change?

     @staticmethod
     def paginate(
-        request: R[CP, CT],
+        request: Union[R[CP, List[RT]], R[CP, CT]],
         page: int = 1,
         per_page: int = 100,
         map_func: Optional[Callable[[Response[CT]], List[RT]]] = None,
         *args: CP.args,
         **kwargs: CP.kwargs,
     ) -> Paginator[RT]:
         return Paginator(request, page, per_page, map_func, *args, **kwargs)  # type: ignore
yanyongyu commented 1 year ago

Everything is OK in my VSCode with Pylance/Pyright :)

image

or with map_func

image

netomi commented 1 year ago

I tested your change in PyCharm, its basically the same error as before. As I said it might be a bug in the relevant analyser. I checked their issue tracker, and there there seem to be a couple of open issues. This also is a quite complex case.

yanyongyu commented 1 year ago

Thanks to @RF-Tar-Railt, this issue has been confirmed and tracked by Jetbrains in PY-61022 & PY-61023. This issue maybe fixed in pycharm feature release.