Cog-Creators / Red-DiscordBot

A multi-function Discord bot
https://docs.discord.red
GNU General Public License v3.0
4.73k stars 2.3k forks source link

[Mutes] Duration should take the first value provided #6274

Closed EternalllZM closed 5 months ago

EternalllZM commented 9 months ago

What Red version are you using?

3.5.5

Cog name

Mutes

Command name

mute

What did you expect to happen?

[p]mute <user_id> 1h spamming 3 copypastas within 2 hours.

The first time mentioned is 1h, which means the intended mute time is 1 hour.

What actually happened?

Our example: [p]mute <user_id> 1h spamming 3 copypastas within 2 hours.

If you mention a time, even after one has been specified, it will use the last time mentioned. In this case, 2 hours is used as the duration instead of the intended 1h.

Per mutes help screen, the following examples are provided:

[p]mute @member1 @member2 spam 5 hours
[p]mute @member1 3 days

I would argue that the first mention of a time should be used as the base for the command's duration. In a reason you might need to explain that a certain amount of bad behavior took place within a time duration.

For further explanation, the mute help provides this: [p]mute <users...> [time_and_reason]

Being picky, time is listed before reason. Therefore, the line of thinking is that you provide a time before the reason, even if it does not matter when reviewing listed examples.

How can we reproduce this error?

  1. Load mutes cog.
  2. Execute a mute command as such: [p]mute <user_id> 1h spamming 3 copypastas within 2 hours
  3. Mute will set duration as 2 hours instead of the 1h specified first.

Anything else?

This would not effect the ability to format mutes in the second example, except it would reverse the behavior (second time is now overwritten by first time mentioned). [p]mute @member1 @member2 spam 5 hours

If no other time is passed before the 5 hours in the command, this would function just the same. Mostly this bug report is looking to change the behavior to accept the first mention of a time, rather than the last as it would make more sense.

Flame442 commented 9 months ago

It's expected behavior that the time can be matched anywhere in the string, however it might not be expected behavior to use the last time in the string. Currently, it loops over all the matches front to back, and overrides the assumed time with each valid match, meaning the last time entered will be chosen. It might make sense to break on valid match, to ensure the first time entered is the one that is used. https://github.com/Cog-Creators/Red-DiscordBot/blob/3fd0afd87daf5e91ee3f6bee245e12fa3af86b86/redbot/cogs/mutes/converters.py#L50-L54

Flame442 commented 5 months ago

Additionally, it seems like the regex may be overly ambitious in matching the single letter versions of times in some cases, such as 5 warnings being interpreted as 5 w.

Repro:

from redbot.cogs.mutes.converters import MuteTime

return await MuteTime().convert(ctx, "7 days Rule Violation - User has accumulated 5 warnings related to misuse of channels")
{'duration': datetime.timedelta(days=42), 'reason': 'Rule Violation - User has accumulated arnings related to misuse of channels'}