Skip to content

Love command checks#806

Merged
Xithrius merged 8 commits into
mainfrom
love-command-checks
Aug 16, 2021
Merged

Love command checks#806
Xithrius merged 8 commits into
mainfrom
love-command-checks

Conversation

@ChrisLovering
Copy link
Copy Markdown
Member

@ChrisLovering ChrisLovering commented Aug 10, 2021

Description

This makes a few changes to the .love command to make it slightly less creepy, more consistent with other lovefest commands and also makes it give consistent output, regardless of input order.

The changes I have made are:

  • Added and in_month check, to ensure the command can only run during February (valentine season)
  • Require that both members have opted into the love fest event (by checking roles)
  • Sort the members to be checked before hashing, to ensure symmetrical results.
  • Removed some now un-used checks since we don't accept arbitrary strings as input

Did you:

This ensure the valentines themed command can only be ran during the valentines season.
This ensure that only uses that have opted into the love fest event can actually be used as part of this command.
This ensures the same result for same input, regardless of order
Since we update how the command behaves, by not allow arbitrary stings, and being symmetrical, we should reflect this in the docstring.
Now that we don't allow arbitrary strings, we can simplify the input validation/modification we do.
Copy link
Copy Markdown
Contributor

@Akarys42 Akarys42 left a comment

Choose a reason for hiding this comment

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

Sorry 😔

Comment thread bot/exts/valentines/lovecalculator.py Outdated
…check fails

Co-authored-by: Matteo Bertucci <matteobertucci2004@gmail.com>
@Akarys42 Akarys42 dismissed their stale review August 10, 2021 14:18

Requested changes applied

Comment thread bot/exts/valentines/lovecalculator.py Outdated
Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

Could add a footer to the embed sent saying you can unsubscribe with .lovefest unsub, but you could also not do that, I don't' mind.

Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

lgtm

This skips the role check if the author is running the love calc command on themselves. We still want to check the the user running against has the role, along with if the author is running it against 2 other people.

i have also included info on how to get the lovefest role in the error embed.
@ChrisLovering ChrisLovering added the status: needs review Author is waiting for someone to review and approve label Aug 14, 2021
@Xithrius Xithrius added area: backend Related to internal functionality and utilities category: valentines type: enhancement Changes or improvements to existing features labels Aug 16, 2021
Copy link
Copy Markdown
Contributor

@Xithrius Xithrius left a comment

Choose a reason for hiding this comment

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

Yes.

@Xithrius Xithrius merged commit f58e898 into main Aug 16, 2021
@Xithrius Xithrius deleted the love-command-checks branch August 16, 2021 03:06
@Xithrius Xithrius removed the status: needs review Author is waiting for someone to review and approve label Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: backend Related to internal functionality and utilities type: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants