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

[Warnings]Make it possible to add reason with unwarn #3490

Merged
merged 11 commits into from Feb 14, 2020
Merged

[Warnings]Make it possible to add reason with unwarn #3490

merged 11 commits into from Feb 14, 2020

Conversation

Dav-Git
Copy link
Contributor

@Dav-Git Dav-Git commented Feb 2, 2020

Type

Bugfix
Enhancement

[X] New feature

Description of the changes

This closes #3093 .
This overwrites #3441 because I'm stupid and deleted the fork.

@Dav-Git Dav-Git requested a review from palmtree5 as a code owner Feb 2, 2020
@jack1142 jack1142 added the Type: Enhancement label Feb 2, 2020
Drapersniper
Drapersniper previously requested changes Feb 3, 2020
redbot/cogs/warnings/warnings.py Outdated Show resolved Hide resolved
redbot/cogs/warnings/warnings.py Outdated Show resolved Hide resolved
redbot/cogs/warnings/warnings.py Outdated Show resolved Hide resolved
redbot/cogs/warnings/warnings.py Outdated Show resolved Hide resolved
Dav-Git and others added 3 commits Feb 3, 2020
Co-Authored-By: Draper <27962761+Drapersniper@users.noreply.github.com>
Co-Authored-By: Draper <27962761+Drapersniper@users.noreply.github.com>
@Dav-Git Dav-Git requested a review from Drapersniper Feb 3, 2020
Copy link
Contributor

@Drapersniper Drapersniper left a comment

This is a review for the Code and not functionality:
LGTM (Can't check functionality atm)

msg = _("That is not a registered reason.")
if custom_allowed:
reason_type = {"description": reason}
elif (
Copy link
Contributor

@Drapersniper Drapersniper Feb 3, 2020

Choose a reason for hiding this comment

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

Actually stating the valid reasons, when custom_allowed is disallowed and reason.lower() not in registered_reasons would be more user friendly (something along the lines of "Please use one of the following: ...")

Right now if reason.lower() not in registered_reasons and not custom_allowed this just fails silently

Copy link
Contributor Author

@Dav-Git Dav-Git Feb 3, 2020

Choose a reason for hiding this comment

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

Will look at this when I return home later today

Copy link
Contributor Author

@Dav-Git Dav-Git Feb 3, 2020

Choose a reason for hiding this comment

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

This would then require to be edited in the warn command as well. My knowledge in string formatting is limited (everyone tells me my code is to slow) that I'd rather leave that up to someone else as it also doesn't really fit in this PR in my opinion. If you really want to see this included I'll need some guidance via discord.

I'm guessing that reason_lower is a list of strings that in one way ore another can be concatinated and passed to an embed which is then sent to the user. If we exceed the embed limit with the amount of registered reasons that's going to require some logic I don't know how to code and I'm not really sure if it's worth it at that point. Leaving it with just the instructions on how to enable custom warnings seems reasonable to me.

@Dav-Git
Copy link
Contributor Author

@Dav-Git Dav-Git commented Feb 6, 2020

Are they supposed to not report anything? Do I need to do something?

@Dav-Git Dav-Git requested a review from Drapersniper Feb 7, 2020
else:
reason_type = registered_reasons[reason.lower()]
if reason_type is None:
return
Copy link
Contributor

@Drapersniper Drapersniper Feb 7, 2020

Choose a reason for hiding this comment

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

I still think we should return a user friendly message when a valid reason is not present, just silently doing nothing is very confusing imo

Copy link
Contributor

@Drapersniper Drapersniper Feb 7, 2020

Choose a reason for hiding this comment

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

If another QA/Core-dev disagrees let me know and i'll review this as it is.

Copy link
Member

@jack1142 jack1142 Feb 7, 2020

Choose a reason for hiding this comment

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

I do agree we shouldn't fail silently, though I don't think we should list all registered reasons, especially not in scope of this PR.

@jack1142
Copy link
Member

@jack1142 jack1142 commented Feb 7, 2020

I still think that registered warn reasons shouldn't be used for unwarn - they will simply not make sense as unwarn reasons. I think allowing any text as unwarn reason is enough but I guess there's also the option of registering unwarn reasons.

@Dav-Git
Copy link
Contributor Author

@Dav-Git Dav-Git commented Feb 7, 2020

I think just removing the use of registered reasons for unwarn is the way forward with this. You guys let me know if that's what you want me to do.

…y if the command author was an admin, guild owner or bot owner.
@Dav-Git Dav-Git requested a review from Drapersniper Feb 9, 2020
Copy link
Contributor

@Drapersniper Drapersniper left a comment

Okay now that we return a message when reason_type is None I’m more okay with this. Although jack made a point And not sure how strongly he feels about this one @jack1142 do you think removing the usage of the built in reason type is important enough here?

@jack1142
Copy link
Member

@jack1142 jack1142 commented Feb 9, 2020

@Dav-Git after additional discussion, we decided that [p]uwarn should just allow any reason.

@@ -433,14 +462,15 @@ def __init__(self, bot: Red):
await member_settings.total_points.set(current_point_count)
user_warnings.pop(warn_id)
try:
reason_msg = "{description}".format(description=reason_type["description"])
Copy link
Member

@jack1142 jack1142 Feb 9, 2020

Choose a reason for hiding this comment

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

This can just be (though this part might not even exist after allowing any reason):

Suggested change
reason_msg = "{description}".format(description=reason_type["description"])
reason_msg = reason_type["description"]

@Dav-Git
Copy link
Contributor Author

@Dav-Git Dav-Git commented Feb 9, 2020

Sounds good to me. I'll remove it for both commands. Maybe the commands to set the warn reasons are pending removal too though? Especially since the reasons then no longer server a purpose.

@Dav-Git
Copy link
Contributor Author

@Dav-Git Dav-Git commented Feb 9, 2020

Correction, I misread that, allowing any reason for just unwarn

@Dav-Git Dav-Git requested review from Drapersniper and jack1142 Feb 9, 2020
redbot/cogs/warnings/warnings.py Outdated Show resolved Hide resolved
@Dav-Git Dav-Git requested a review from jack1142 Feb 9, 2020
Copy link
Member

@jack1142 jack1142 left a comment

Looks like I'm gonna review this PR after all, only one change should be needed here before approval :)

redbot/cogs/warnings/warnings.py Outdated Show resolved Hide resolved
@jack1142 jack1142 self-assigned this Feb 9, 2020
@jack1142 jack1142 added this to the 3.3.2 milestone Feb 9, 2020
Co-Authored-By: jack1142 <6032823+jack1142@users.noreply.github.com>
Copy link
Member

@jack1142 jack1142 left a comment

LGTM

@mikeshardmind mikeshardmind merged commit e796999 into Cog-Creators:V3/develop Feb 14, 2020
5 checks passed
@Dav-Git Dav-Git deleted the patch-unwarn-reason branch Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants