-
-
Notifications
You must be signed in to change notification settings - Fork 718
Preserve Invocation Context #3377
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
base: main
Are you sure you want to change the base?
Preserve Invocation Context #3377
Conversation
bot/decorators.py
Outdated
files=[PasteFile(content=ctx.message.content, lexer="markdown")], | ||
http_session=session, |
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.
It looks like these lines are indented an extra level.
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.
Fixed.
bot/decorators.py
Outdated
log.exception( | ||
"Failed to upload message %d in channel %d to paste service when redirecting output", | ||
ctx.message.channel.id, ctx.message.id | ||
) |
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.
This one line should be indented one level less.
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.
Fixed.
bot/decorators.py
Outdated
msg += "Here's the output of " | ||
|
||
if paste_link: | ||
msg += f"[your command]({paste_link}):" | ||
else: | ||
msg += "your command:" |
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.
This could be one line using a ternary condition.
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.
Fixed
@jb3, am I correct in my understanding that this copies messages from the server to our pastebin, possibly unexpectedly? |
else: | ||
msg += "your command:" | ||
|
||
await ctx.send(msg) |
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.
Would this potentially send an empty message?
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.
Resolved with the ternary changes.
bot/decorators.py
Outdated
if ping_user: | ||
await ctx.send(f"Here's the output of your command, {ctx.author.mention}") | ||
msg += f"{ctx.author.mention}, " |
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.
Why are we changing the order and putting the mention first?
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.
Do you want it changed? It can be done.
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.
I don't have a strong preference, but I figure we should leave it alone if there is no compelling reason to change it.
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.
Yeah this was probably something I changed inadvertedly when I wrote something on mobile, it can be changed back.
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.
Should I change and commit then? Or should we just leave it as is?
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.
Change it back, there's no reason to alter it from what already exists.
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.
Fixed.
Yes, but with it being a bot command I'm less worried about that. Previously we've worried about that when things such as automatically uploading unformatted code have been mentioned, or when we are talking about uploading messages which are not direct interactions with the bot. In this case, it's reasonable to temporarily upload after a user has directly used our features (bear in mind we automatically upload the results if they are too long). |
Perhaps we can send the user the link to delete the paste? You can largely copy the code here: bot/bot/exts/utils/attachment_pastebin_uploader.py Lines 130 to 133 in cb4401b
|
Head branch was pushed to by a user without write access
Added. |
bot/decorators.py
Outdated
content= | ||
f"Your command output was redirected to <#{Channels.bot_commands}>. [Click here](<{paste_response.removal}>) to delete the automatically uploaded copy of your original command." |
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.
When a string is too long, the preferred format is like this
content = (
"Lorem ipsum dolor sit amet, consectetur "
f"adipiscing {elit}, sed do eiusmod tempor "
"incididunt ut labore et dolore magna aliqua."
)
This leverages implicit string concatenation. Note that segments using f-string need to start with an f. The trailing spaces at the end of each line are literal single spaces that are not line breaks.
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.
Yeah, Just saw the lint errors, pushed a commit.
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.
Formatting and wording.
Co-authored-by: Joe Banks <[email protected]>
Co-authored-by: Joe Banks <[email protected]>
bot/decorators.py
Outdated
" copy of your original command." | ||
) | ||
except discord.Forbidden: | ||
log.warning("Redirect output: Failed to send DM to user. Forbidden.") |
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.
This probably isn't a warning really, it's expected behaviour that we are catching.
log.warning("Redirect output: Failed to send DM to user. Forbidden.") | |
log.info( | |
"Failed to DM %s with redirected command paste removal link, user has bot DMs disabled", | |
ctx.author.name | |
) |
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.
Done.But this was your suggestion.
Co-authored-by: Joe Banks <[email protected]>
Added:
Closes #3376