-
-
Notifications
You must be signed in to change notification settings - Fork 41
feat: defer processing message instead of reply #481
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
Conversation
Reviewer's Guide by SourceryThis pull request modifies the image processing functionality in the 'deepfry' command to use deferred responses instead of immediate replies, improving the user experience for longer operations. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Ow0cast - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider updating the logging statements to accurately reflect the new deferred processing behavior.
- It might be beneficial to add error handling for the image processing operation to gracefully handle any failures.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
tux/cogs/fun/imgeffect.py
Outdated
| # say that the image is being processed | ||
| logger.info("Processing image...") | ||
| await interaction.response.send_message("Processing image...") | ||
| await interaction.response.defer("Processing image...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider removing the message from defer()
The defer() method doesn't accept a message parameter. It's used to acknowledge the interaction without sending a response. You can remove the "Processing image..." message from here.
| await interaction.response.defer("Processing image...") | |
| await interaction.response.defer() |
tux/cogs/fun/imgeffect.py
Outdated
| file = discord.File(arr, filename="deepfried.jpg") | ||
| # edit message with image | ||
| await interaction.followup.send(content="Here is your deepfried image:", file=file) | ||
| await interaction.response.edit_message(content="Here is your deepfried image:", file=file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Use followup.send instead of response.edit_message after deferring
When using defer(), you should use interaction.followup.send() to send the final response, not edit_message(). There's no initial message to edit after deferring, which could lead to an error.
|
@Ow0cast has this been tested or does this need testing in the future please follow the pr template |
|
im going to hit you with a can of sprite |
|
blame github it doesn't automatically use it |
|
odd, it does from my experience so ill have to take a look at that but does it need to be tested? |
|
it hasnt been tested but it should work according to the docs but the dumb ai bot thinks otherwise regardless shouldn't hurt to test |
|
This is a primer on the subject: https://gist.github.com/cibere/7e1356575780e716d2e3a23ea2bcf6da |
electron271
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A command invoke error occurred: Command 'deepfry' raised an exception: TypeError: InteractionResponse.defer() takes 1 positional argument but 2 were given
Summary by Sourcery
Defer the initial processing message and edit the message with the final image instead of sending a follow-up message, streamlining the interaction flow.
Enhancements: