Implemented optional duration parameter in slowmode command#3331
Implemented optional duration parameter in slowmode command#3331mbaruh merged 10 commits intopython-discord:mainfrom
Conversation
jb3
left a comment
jb3
left a comment
There was a problem hiding this comment.
One minor comment, though if others disagree then let me know, it is more a nit based on me not wanting to add dependence onto intricacies of Redis and hoping multiple keys exist referring to the same slowmode delay etc.
| if await self.slowmode_expiration_cache.contains(channel.id): | ||
| expiration_time = await self.slowmode_expiration_cache.get(channel.id) |
There was a problem hiding this comment.
| if await self.slowmode_expiration_cache.contains(channel.id): | |
| expiration_time = await self.slowmode_expiration_cache.get(channel.id) | |
| expiration_time = await self.slowmode_expiration_cache.get(channel.id, None) | |
| if expiration_time is not None: |
| expiration_time = await self.slowmode_expiration_cache.get(channel.id) | ||
| expiration_timestamp = discord_timestamp(expiration_time, TimestampFormats.RELATIVE) | ||
| await ctx.send( | ||
| f"The slowmode delay for {channel.mention} is {humanized_delay} and expires in {expiration_timestamp}." |
There was a problem hiding this comment.
| f"The slowmode delay for {channel.mention} is {humanized_delay} and expires in {expiration_timestamp}." | |
| f"The slowmode delay for {channel.mention} is {humanized_delay} and expires {expiration_timestamp}." |
The "in" should be already included in a relative timestamp if I'm not mistaken
Also, should this specify what happens when the slowmode expires? e.g "and will revert to {humanized_original_delay} {expiration_timestamp}"
| if channel.id in COMMONLY_SLOWMODED_CHANNELS: | ||
| log.info(f"Recording slowmode change in stats for {channel.name}.") | ||
| self.bot.stats.gauge(f"slowmode.{COMMONLY_SLOWMODED_CHANNELS[channel.id]}", slowmode_delay) | ||
| if not slowmode_delay <= SLOWMODE_MAX_DELAY: |
There was a problem hiding this comment.
| if not slowmode_delay <= SLOWMODE_MAX_DELAY: | |
| if slowmode_delay > SLOWMODE_MAX_DELAY: |
| ctx: Context, | ||
| channel: MessageHolder, | ||
| delay: DurationDelta | Literal["0s", "0seconds"], | ||
| duration: DurationDelta | None = None |
There was a problem hiding this comment.
Use Duration instead of DurationDelta, which handles the conversion to datetime for you (and change the variable name to something like expiry to make it less confusing with delay).
| slowmode_duration = time.relativedelta_to_timedelta(duration).total_seconds() | ||
| humanized_duration = time.humanize_delta(duration) | ||
|
|
||
| expiration_time = datetime.now(tz=UTC) + timedelta(seconds=slowmode_duration) |
There was a problem hiding this comment.
This section is then simplified with the usage of Duration.
| f"{ctx.author} tried to set the slowmode delay of #{channel} to {humanized_delay}, " | ||
| "which is not between 0 and 6 hours." | ||
| f"{ctx.author} set the slowmode delay for #{channel} to" | ||
| f"{humanized_delay} which expires in {humanized_duration}." |
There was a problem hiding this comment.
| f"{humanized_delay} which expires in {humanized_duration}." | |
| f"{humanized_delay} which expires {humanized_duration}." |
Same here, can specify what it's going to revert to.
| await channel.edit(slowmode_delay=slowmode_delay) | ||
| await ctx.send( | ||
| f"{Emojis.check_mark} The slowmode delay for {channel.mention}" | ||
| f" is now {humanized_delay} and expires in {expiration_timestamp}." |
| await channel.send( | ||
| f"{Emojis.check_mark} A previously applied slowmode has expired and has been reverted to {slowmode_delay}." | ||
| ) |
There was a problem hiding this comment.
Slowmode changes are usually silent. Unless the mods agreed that this is what they want I would send this message to #mods and/or #mod-log
There was a problem hiding this comment.
Looks like mod-log will already pick up when the slowmode delay is modified and sends it as an embed. Should I also send the above to that channel?
There was a problem hiding this comment.
And if i send it to mod-log, should get_or_fetch_channel be used here also? Should the mod-log channel be defined as an instance attribute inside init or is it fine to define it inside of the command provided that get_or_fetch_channel is used?
There was a problem hiding this comment.
No if it's automatically picked up then great. I'd just send this to #mods
| async def _revert_slowmode(self, channel_id: int) -> None: | ||
| original_slowmode = await self.original_slowmode_cache.get(channel_id) | ||
| slowmode_delay = time.humanize_delta(seconds=original_slowmode) | ||
| channel = self.bot.get_channel(channel_id) |
There was a problem hiding this comment.
I would use get_or_fetch_channel here
| if channel is None: | ||
| channel = ctx.channel | ||
| if await self.slowmode_expiration_cache.contains(channel.id): | ||
| await self.slowmode_expiration_cache.delete(channel.id) | ||
| await self.original_slowmode_cache.delete(channel.id) | ||
| self.scheduler.cancel(channel.id) |
There was a problem hiding this comment.
Shouldn't it all be already handled in set_slowmode?
…ches into one object; update and tidy related tests.
mbaruh
left a comment
mbaruh
left a comment
There was a problem hiding this comment.
Sorry it took me so long to respond, solid PR overall. I made a small improvement to make the code a bit cleaner (which fixed a small bug as a side effect)
Looks much nicer, thanks! Just out of curiosity, what was the bug? |
|
you used .partition in the get function which returns three values, but unpacked into only two variables. In the other functions you used .split and [0] so there were no issues there |
4 participants
Closes #3327.
Implemented an optional
durationargument for the slowmode command. If the duration argument is present, the intended expiration datetime and the original slowmode interval are saved in redis cache. After the duration is elapsed, the original slowmode interval is grabbed from redis and restored to the channel.The expiration datetime is cached so that it can persist bot restart.