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

feat: auto-generate overloads for Client.wait_for #1017

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

shiftinv
Copy link
Member

@shiftinv shiftinv commented Apr 28, 2023

Summary

Part 1/? of event QoL work, adds just a few overloads (106, to be exact) to Client.wait_for, one for each event.
This allows for correct typing of the check parameter and the return type, particularly when used with the Event enum (see below).
These are being auto-generated through yet another custom libcst module. While certainly not ideal, there isn't a better way to do this with Python's current typing system due to several reasons.

We may want to consider merging overloads with the same return types, I'm not sure if that affects pyright performance in any significant way.
That one big lookup table is a separate module as part of the library, since it'll be reused and expanded in future PRs with runtime usage.

Some other things:

Fallback for str

One of the overloads takes an unspecific str event. This is meant to be the fallback overload for anyone using the existing event infrastructure in the lib for custom events, since doing that would otherwise run into typing issues:

disnake/disnake/client.py

Lines 2742 to 2751 in a52790d

@overload
@_overload_with_events
def wait_for(
self,
event: str,
*,
check: Optional[Callable[..., bool]] = None,
timeout: Optional[float] = None,
) -> Coroutine[Any, Any, Any]:
...

This, however, means that incorrectly typing a library event handler also falls back to this, since it's the most generic one - i.e. passing a wrong check type for .wait_for("message") will not show any errors. This is not the case with Event.message, which will correctly show a typing error if necessary.

But what about the other event methods?

No :) Well, maybe.
I partially implemented this, but decided to remove it again; it may or may not affect the performance of pyright(/mypy), and the library typing is already complex enough.
Reason for choosing wait_for in particular is that it's usually used together with check=lambda x, y, z: ..., and you can't add type annotations to inline lambdas.

Somewhat depends on #1016 due to tests.

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running pdm lint
    • I have type-checked the code by running pdm pyright
  • This PR fixes an issue
  • This PR adds something new (e.g. new method or parameters)
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@shiftinv shiftinv added t: enhancement New feature s: needs review Issue/PR is awaiting reviews t: refactor/typing/lint Refactors, typing changes and/or linting changes labels Apr 28, 2023
@shiftinv shiftinv force-pushed the feature/codemod-wait-for-overloads branch from 8d27cad to ee43582 Compare May 2, 2023 15:31
@shiftinv shiftinv added this to the disnake v2.10 milestone May 24, 2023
Base automatically changed from tests/event-listeners to master May 27, 2023 00:21
@onerandomusername
Copy link
Member

@shiftinv would you please solve conflicts?

@shiftinv shiftinv force-pushed the feature/codemod-wait-for-overloads branch 2 times, most recently from b3b0cef to cf276c7 Compare June 13, 2023 13:00
@shiftinv
Copy link
Member Author

@shiftinv would you please solve conflicts?

done, rebased away the commits from the base PR and updated to master 👍

@shiftinv shiftinv force-pushed the feature/codemod-wait-for-overloads branch from a171fab to 61f85fb Compare June 13, 2023 13:11
@EmmmaTech
Copy link
Contributor

I hate the concept of putting a ton of overloads in the main client.py file. It makes it way more bloated than it needs to be. Can't wait_for and its overloads be put in a mixin in a separate file?

@shiftinv
Copy link
Member Author

I hate the concept of putting a ton of overloads in the main client.py file. It makes it way more bloated than it needs to be. Can't wait_for and its overloads be put in a mixin in a separate file?

It's not ideal either way, yeah. This still seems to be the least awful best solution, given we unfortunately can't just have a wait_for<T extends keyof typeof Event>(event: T, check: (...args: EventData[T]) => boolean) or something else that would avoid all these overloads in the first place :/
I'd like to avoid mixins as much as possible due to them being mostly incompatible with type checking, moving wait_for into a separate mixin/file would require additional # type: ignores.

@EmmmaTech
Copy link
Contributor

I'd like to avoid mixins as much as possible due to them being mostly incompatible with type checking, moving wait_for into a separate mixin/file would require additional # type: ignores.

How come?

@EQUENOS
Copy link
Member

EQUENOS commented Sep 21, 2023

I hate the concept of putting a ton of overloads in the main client.py file. It makes it way more bloated than it needs to be. Can't wait_for and its overloads be put in a mixin in a separate file?

Instead of mixins maybe .pyi files could be used to achieve this? I don't know much about those though

@elenakrittik
Copy link
Contributor

elenakrittik commented Sep 21, 2023

I hate the concept of putting a ton of overloads in the main client.py file. It makes it way more bloated than it needs to be. Can't wait_for and its overloads be put in a mixin in a separate file?

Instead of mixins maybe .pyi files could be used to achieve this? I don't know much about those though

Not an expert on the topic either, but i heard that .pyi files are preferred over source code by type checkers, which means if we're going to supply a .pyi file we'll have to type hint the entirety of client.py there (since for TCs can't consume both stubs and source hints), which sounds like another PR to drop source hints altogether.

(mypy reference: https://mypy.readthedocs.io/en/stable/stubs.html#creating-a-stub - "If a directory contains both a .py and a .pyi file for the same module, the .pyi file takes precedence.")

EDIT: Forget what i said, it seems like in theory there is a way to make "partial" stubs: https://peps.python.org/pep-0561/#partial-stub-packages

@shiftinv
Copy link
Member Author

which means if we're going to supply a .pyi file we'll have to type hint the entirety of client.py there

Yea, not something we want to do. Creating "partial" stubs like you mentioned would be cool, but that mechanism only applies at the file/module level and not the class/func level, which would be needed here.

@shiftinv shiftinv modified the milestones: disnake v2.10, disnake v2.11 Aug 9, 2024
Comment on lines +68 to +69
# n.b. this one isn't ideal, but there's no way to prevent type-checkers from using the fallback in this case
_ = assert_type(client.wait_for("guild_join", check=lambda: True), Coroutine[Any, Any, Any])
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, an overload like

@overload
async def wait_for(
    self,
    event: Literal["guild_join", "ready", ...] | Event,
    *,
    check: Callable[..., Any],
    timeout: Optional[float] = None,
) -> Never: ...

Could work, but the Literal would be huge, and it's non-obvious what that signature means/why it got matched

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, interesting idea

And yeah, communicating this clearly to the user is tough. I could see InvalidCheckType = Never (or w/e) maybe being an option, which would look something like this, but it's not pretty:
image

fwiw, I think ultimately the approach in this PR is somewhat flawed, but it's the best we can do for now, sadly. In an ideal world (3.0?), every event would be a dataclass/namedtuple, and event names + overloads would be a thing of the past.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: needs review Issue/PR is awaiting reviews t: enhancement New feature t: refactor/typing/lint Refactors, typing changes and/or linting changes
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

6 participants