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

[docs] add wait_for guide #7709

Open
wants to merge 14 commits into
base: docs/guide
Choose a base branch
from
Open

Conversation

pikaninja
Copy link
Contributor

Summary

This pr adds the wait_for guide in the documentation. This was done in conjunction with HayBaleAv. I believe there's a few odd wording in this, any feedback would be appreciated

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • 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, ...)

@pikaninja pikaninja changed the title add wait_for guide [docs] add wait_for guide Mar 18, 2022
@Rapptz Rapptz added the guide This relates to the discord.py guide label Mar 19, 2022
@Rapptz Rapptz mentioned this pull request Mar 19, 2022
23 tasks
@mirnovov
Copy link
Contributor

It might be a good idea to include basic instructions on using wait_for for multiple things simultaneously, as in the multiple wait_for tag on the discord server. Though of course, including the proviso that it's a more advanced topic when mentioning it.

@pikaninja
Copy link
Contributor Author

It might be a good idea to include basic instructions on using wait_for for multiple things simultaneously, as in the multiple wait_for tag on the discord server. Though of course, including the proviso that it's a more advanced topic when mentioning it.

i did consider adding this, but i felt it was a bit too jank and confusing to add to the guide, idk though

Copy link
Contributor

@mysistersbrother mysistersbrother left a comment

Choose a reason for hiding this comment

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

some wording changes for extra clarity and typo fixes

Copy link
Contributor

@numbermaniac numbermaniac left a comment

Choose a reason for hiding this comment

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

A couple of small suggested changes.

Copy link
Contributor

@AbstractUmbra AbstractUmbra left a comment

Choose a reason for hiding this comment

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

I have added a handful of comments below.

Since we are now a strongly typed library I believe our examples should be typed too.

Copy link
Contributor

@numbermaniac numbermaniac left a comment

Choose a reason for hiding this comment

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

Just some quick fixes to the new text added to the guide.

Co-authored-by: numbermaniac <[email protected]>
reaction: discord.Reaction, user: discord.User = await client.wait_for('reaction_add', check = check)
await message.channel.send(f'You reacted with {reaction.emoji}!')

Notice the ``reaction_add`` event, unlike the ``message`` event, takes 2 arguments. Thus, the check function takes the same arguments as that, ``reaction, user``.
Copy link

Choose a reason for hiding this comment

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

You should emphasize that it takes the same arguments as the event.

Copy link
Contributor

@ika2kki ika2kki left a comment

Choose a reason for hiding this comment

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

review for pika~
feels free to ignore anything.
aside from comments, something i would also like to see is an example on waiting for multiple events with asyncio.wait in an "Advanced" section or similar. its very niche however so not needed

We pass the timeout in seconds to the ``timeout`` kwarg of wait_for. If the wait_for does not complete successfully within the given time it will be terminated and :class:`asyncio.TimeoutError` will be raised.
.. warning::

avoid using wait_for within a loop to catch events of a specific type. Due to the async nature of discord.py, events may be fired between loops, causing your wait_for to miss the event dispatch.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
avoid using wait_for within a loop to catch events of a specific type. Due to the async nature of discord.py, events may be fired between loops, causing your wait_for to miss the event dispatch.
Avoid calling :meth:`Client.wait_for` within a loop to catch events. Due to the nature of async IO, events may be fired between loops, causing the loop to miss events.

i like the warning but i dont like how it doesnt give solution lole. the typical approach with this is to append event arguments in a list from inside the predicate and then return True whenever u want it to stop (an example could be like, queuing up to 10 people to play russian roulete or something and i think it would be a funny example or something you can put under a "Advanced usage" subtopic).

Copy link
Contributor

@AbstractUmbra AbstractUmbra left a comment

Choose a reason for hiding this comment

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

Changes supplied in the review. :)

Some other thoughts are, could we add a bit more complex examples? Like waiting for some non-common event (so not message or reaction, but maybe something like a typing event?) that showcases the difference in passed check parameters and whatnot, and maybe showcase a raw one to mention the raw typed payload(s) in comparison to complete objects?


Examples
~~~~~~~~~~~~~~~~~~~~
Wait for reaction
Copy link
Contributor

Choose a reason for hiding this comment

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

Likely nitpicky but I feel this could link to the documentation pages for on_reaction_add and mention the raw variant too.

No need for an explanation of raw vs not, just linking them would be good so people can "avoid" the message needing to be in cache to catch a full Reaction.

@AbstractUmbra
Copy link
Contributor

Oh, one last thing for now.

Could the file be moved to docs/guide/topics/wait_for.rst to match the rest?

Copy link
Contributor

@tailoric tailoric left a comment

Choose a reason for hiding this comment

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

Mostly some formatting problems that I've noticed when building the docs for myself.


Closing Remarks
~~~~~~~~~~~~~~~~~~~~
``wait_for`` is a powerful tool used often to wait for responses in code. The examples above show only 2 types of ``wait_for``, reactions and messages, but you can wait for any event! A full list of events can be seen here: :ref:`event reference <discord-api-events>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

While they are powerful I think that it should be mentioned that Buttons and Modals can be also used for similar functionality that provide better UX for users instead and could/should be considered of waiting for a message or reaction.

However I can also understand that this may be out of scope for the wait_for guide and probably better to be added to the views/modal guide instead.

Also I am a bit struggling to find good use cases on waiting for other events than reaction or on_message events, especially ones that aren't superseded by UI components. Because I think the examples should show some functionality that is not possible with those, however again that is just a major personal nitpick mostly but I wanted to mention it.

Only one example I can think of could be having a wait_for for the voice_state_update event to join with a user who invoked a command that needs someone to be in voice before doing something. Roughly working like so:

  1. Command should play something in the voice channel the user is in
  2. check if user is in voice if yes join and play sound
  3. else send a message asking them to join a voice channel
  4. wait_for user to join a voice channel with timeout
  5. play sound / file after user joined.

It is a bit convoluted but also a bit more of an involved example that could show off the capability of wait_for

Copy link
Contributor

@ika2kki ika2kki left a comment

Choose a reason for hiding this comment

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

sphinx stuff mostly.
mentioned earlier but one more example i wanna see is waiting on events of a different type at the same time (alongside waiting for multiple events of the same sequentially) - can do a confirmation prompt, where you can either wait for a yes/no message or reaction.

Comment on lines +44 to +46
In this example, ``wait_for`` will only accept the incoming message when the predicate returns ``True``.
The arguments passed to the predicate match the signature of the corresponding event.
For example, when defining a message event handler we do ``async def on_message(message)`` - a predicate would similarly be defined with ``def check(message)``.
Copy link
Contributor

Choose a reason for hiding this comment

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

add a note that checks have to be synchronous

The arguments passed to the predicate match the signature of the corresponding event.
For example, when defining a message event handler we do ``async def on_message(message)`` - a predicate would similarly be defined with ``def check(message)``.

Likewise, a predicate for `reaction_add` would take `def check(reaction, user)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Likewise, a predicate for `reaction_add` would take `def check(reaction, user)`.
Likewise, when listening for reactions we'd do ``async def on_reaction_add(reaction, user)`` - so a predicate would similarly be defined with ``def check(reaction, user)``.

else:
await msg.reply('hello!')

We pass the timeout in seconds to the ``timeout`` keyword-argument. Timeouts behave like :meth:`asyncio.wait_for`, so we do the usual `try` and `except`, catching :class:`asyncio.TimeoutError`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We pass the timeout in seconds to the ``timeout`` keyword-argument. Timeouts behave like :meth:`asyncio.wait_for`, so we do the usual `try` and `except`, catching :class:`asyncio.TimeoutError`:
We pass the timeout in seconds to the ``timeout`` keyword-argument. Timeouts behave like :meth:`asyncio.wait_for`, so we do the usual `try` and `except`, catching :class:`asyncio.TimeoutError`.

It's common to also include a timeout in addition to a check so our code doesn't potentially end up waiting indefinitely, or to add a limited time window for the author to send a message.

.. code-block:: python3
:emphasize-lines: 2, 3, 4, 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:emphasize-lines: 2, 3, 4, 5
:emphasize-lines: 2-5


.. warning::

Avoid calling :meth:`Client.wait_for` within a loop to catch events. Due to the nature of async IO, events may be fired between loops, causing the loop to miss events. See the example for multiple events.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Avoid calling :meth:`Client.wait_for` within a loop to catch events. Due to the nature of async IO, events may be fired between loops, causing the loop to miss events. See the example for multiple events.
Avoid calling :meth:`Client.wait_for` within a loop to catch events. Due to the nature of async IO, events may be fired between loops, causing the loop to miss events - see the :ref:`example on waiting for multiple events<guide_wait_for_multiple_events>`.

.. code-block:: python3

def check(reaction: discord.Reaction, user: discord.User):
return user == message.author and reaction.message == message # check that the reaction is on a specific message and by a specific user
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return user == message.author and reaction.message == message # check that the reaction is on a specific message and by a specific user
# ensure the reaction is on the relevant message and from the original author
return user == message.author and reaction.message == message

reaction, user = await client.wait_for('reaction_add', check=check)
await message.channel.send(f'You reacted with {reaction.emoji}!')

Notice the ``reaction_add`` event, unlike the ``message`` event, takes 2 arguments. Thus, the check function takes the same arguments as that, ``reaction, user``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Notice the ``reaction_add`` event, unlike the ``message`` event, takes 2 arguments. Thus, the check function takes the same arguments as that, ``reaction, user``.

already mentioned

await message.channel.send(f'You reacted with {reaction.emoji}!')

Notice the ``reaction_add`` event, unlike the ``message`` event, takes 2 arguments. Thus, the check function takes the same arguments as that, ``reaction, user``.
wait_for will follow the same constraints and nuances as the corresponding event, in this case: :meth:`discord.on_reaction_add`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
wait_for will follow the same constraints and nuances as the corresponding event, in this case: :meth:`discord.on_reaction_add`.
:meth:`Client.wait_for` will follow the same constraints and nuances as the corresponding event, in this case, :meth:`discord.on_reaction_add` only triggers for messages in the client's internal cache.

Comment on lines +92 to +103
participants = []

def check(message: discord.Message):
participants.append(message.author)
if len(participants) >= 10:
return True
return False

await client.wait_for('message', check=check)

mentions = ', '.join([user.mention for user in particpants])
await message.channel.send(f'Welcome {mentions}!')
Copy link
Contributor

Choose a reason for hiding this comment

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

if you write this code in a way the guide recommends against:

participants = []
while len(participants) < 10:
    message = await client.wait_for('message', check=check)
    participants.append(message.author)

it isnt actually racey and will gather all the messages appropriately, since wait_for is a sync method at heart and only returns a future, with the internal listeners set up in advance before the event loop ever yields via the await.

if you add a little bit of code in-between iterations with await, like this:

participants = []
while len(participants) < 10:
    message = await client.wait_for('message', check=check)
    participants.append(message.author)
    await message.channel.send(f"{message.author} has joined!")

now its racey so i think the example should offer a solution on this. it can be scheduling send() with asyncio.create_task in the check, or make use of a queue.

feel like its a tough thing to explain in the guide

Comment on lines +86 to +90
Waiting for Multiple Events
+++++++++++++++++++++++++++

.. code-block:: python3
:emphasize-lines: 2, 5, 6, 7
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Waiting for Multiple Events
+++++++++++++++++++++++++++
.. code-block:: python3
:emphasize-lines: 2, 5, 6, 7
.. _guide_wait_for_multiple_events:
Waiting for Multiple Events
+++++++++++++++++++++++++++
A creative approach when it comes to waiting on multiple events is to instead treat the check as a barrier for your code instead of exclusively as a predicate for a single message. For example:
.. code-block:: python3
:emphasize-lines: 2, 5-7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guide This relates to the discord.py guide
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.