-
-
Notifications
You must be signed in to change notification settings - Fork 247
Bemyvalentine patch #1657
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?
Bemyvalentine patch #1657
Conversation
…hannels. python-discord#1477 Adds the ability to send valentines messages publicly or privately to a person. If private, the message is sent in DMs (if possible) and if public it is sent in the current channel that the command was invoked in.
Sets channel "#general" as whitelisted in order to allow testing of invoking the ".bemyvalentine" command and receive the message in the same channel (away from "#sir-lancebot-playground") if "public" was specified in the command.
…d#1477 Adds functionality that instantly deletes messages if "anon" type was specified. Notifications and who is typing will still be sent, but it still provides some anonymity. Also fixes bug that sends the confirmation message to the receiver of the message (should be sender).
Updates the documentation for the new anon/signed attribute to the .bemyvalentine command. Also fixes error in order attributes should appear in.
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.
Thanks for your contribution.
I have not tested or looked into the functionality yet as I notice that CI is failing, but I have a few general comments.
@commands.group(name="bemyvalentine", invoke_without_command=True) | ||
async def send_valentine( | ||
self, ctx: commands.Context, user: discord.Member, *, valentine_type: str | None = None | ||
self, ctx: commands.Context, user: discord.Member, privacy_type: str | None = None, anon: str | None = None, valentine_type: str | None = None |
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.
typying.Literal
can be used to make discord.py check if the argument is valid https://discordpy.readthedocs.io/en/latest/ext/commands/commands.html#typing-literal
This would simplify the logic later on.
@@ -47,6 +47,7 @@ class EnvConfig( | |||
|
|||
|
|||
class _Channels(EnvConfig, env_prefix="channels_"): | |||
general: int = 123123123123 |
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.
Anything changes made just for testing should be reverted (though I'm not really sure why this would be needed, when you can just use a channel that is already configured as whitelisted)
@@ -41,21 +41,39 @@ async def lovefest_role(self, ctx: commands.Context) -> None: | |||
""" | |||
raise MovedCommandError(MOVED_COMMAND) | |||
|
|||
@commands.cooldown(1, 1800, commands.BucketType.user) | |||
# @commands.cooldown(1, 1800, commands.BucketType.user) |
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 assume this was done while testing, but should be reverted now. To keep git history clean, avoid committing temporary changes.
if anon.lower() == "anon": | ||
embed = discord.Embed( | ||
title=f"{emoji_1} {title} {user.display_name} {emoji_2}", | ||
description=f"{valentine} \n **{emoji_2}From an anonymous admirer{emoji_1}**", | ||
color=Colours.pink | ||
) | ||
|
||
else: | ||
embed = discord.Embed( | ||
title=f"{emoji_1} {title} {user.display_name} {emoji_2}", | ||
description=f"{valentine} \n **{emoji_2}From {ctx.author}{emoji_1}**", | ||
color=Colours.pink | ||
) |
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.
There is unnecessary duplication here, you could just have something like
from_user = "an anonymous admirer" if anon.lower() == "anon" else ctx.author
and format that into a single embed call.
@@ -0,0 +1,323 @@ | |||
|
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.
See my comment on another PR - #1656 (comment)
We don't have existing tests on this repo so I'm not sure we want these.
Relevant Issues
Closes #1477
Description
Implements the desired changed to .bemyvalentine. Also adds the feature to let the sender choose if they want to send it anonymously or not.
Did you: