Making DeleteRepeats having a custom value per channel #5607

YannPollet · 2022-03-10T02:44:17Z

Resolves #4721

Description of the changes

I added the functionality to custom the number of allowed repeated messages before deleting it per channel.
The most important change of this feature is that the cache went from a map mapping the last messages from the author in the entire guild to a map mapping the messages of the authors per channel.
I also added in the utils file a method to create a new cache with taking into account the custom values per channel.

Have the changes in this PR been tested?

No, no tests have been added to the test files but the behaviour of the new implementation have been tested with scenario tests (and the tox tests still pass).

Flame442

This is not a complete review. The current implementation of the cache requires too many changes for the rest of the code to be effectively reviewed. I suggest you re-read Jack's comment on the original issue and ensure that you are following his advice in addition to reviewing the comments I have made.

Flame442 · 2022-03-18T22:04:11Z

redbot/cogs/mod/events.py

+        repeats = await self.config.guild(guild).delete_repeats()
+        repeats_channels = await self.config.guild(guild).delete_repeats_channels.all()


The old cache only made config calls when it had not already been filled with data for a particular guild. Yours (inefficiently) does so on every message that is checked. You need to only call from config when needed.

Flame442 · 2022-03-18T22:05:01Z

redbot/cogs/mod/events.py

+        print(guild_cache)
+        print("-----------------------------------------------------------------------")


You should not be leaving any print statements in the code. If there is something that should be displayed in console, you can use the logging module to do so like other parts of the code. This is not such a case however, so these lines should be removed.

Flame442 · 2022-03-18T22:06:13Z

redbot/cogs/mod/events.py

        if not message.content:
            return False


This is not a fault of your PR, but while you are touching this function it is more efficient if this is the first thing checked instead of it being here.

Flame442 · 2022-03-18T22:10:20Z

redbot/cogs/mod/events.py


-        guild_cache[author].append(message.content)
-        msgs = guild_cache[author]
+        guild_cache[str(channel.id)][author].append(message.content)


I honestly cannot tell if it's safe to do this with all the different conditions for how guild_cache can be set up and how this line can be reached.

Could I have some more informations about this? Because I didn't change much about this part of the code except for the dict because I needed a dictionary per channel.

[Your PR code]

Red-DiscordBot/redbot/cogs/mod/utils.py

Lines 20 to 24 in fe92cd8

guild_cache = (

defaultdict(lambda: defaultdict(lambda: deque(maxlen=repeats)))

if repeats != -1

else dict()

)

guild_cache may be a defaultdict[defaultdict[int]], though it is not guaranteed to be one. Thus, I can't tell if blindly accessing guild_cache[str(channel.id)][author] is safe to do from reading the code. This kind of complexity is a bad code smell, so the way this var is created and used should be rewritten to be clearer.

[Current code]

Red-DiscordBot/redbot/cogs/mod/events.py

Lines 24 to 29 in be00a59

guild_cache = self.cache.get(guild.id, None)

if guild_cache is None:

repeats = await self.config.guild(guild).delete_repeats()

if repeats == -1:

return False

guild_cache = self.cache[guild.id] = defaultdict(lambda: deque(maxlen=repeats))

Notice that current code clearly checks whether the guild cache exists, and if it doesn't it clearly creates it, guaranteeing that after the if is done guild_cache will be a defaultdict[int]. I do not have the same confidence reading the code in this PR.

Ok I understand and I already made the changes. It is possible to create a deque with maxlen = 0. Thus these checks aren't useful anymore and have been deleted

Letting you know just in case, you have not pushed any changes to this PR since my review. If you're still working, and you mean that you made the changes locally, that's fine. I just can't see any changes so I can only base what I say on what I can see.

Sorry for the misunderstanding, yes I changed it locally I still have to update my code with these reviews but I will push it in a few days

Flame442 · 2022-03-18T22:12:35Z

redbot/cogs/mod/mod.py

    default_guild_settings = {
        "mention_spam": {"ban": None, "kick": None, "warn": None, "strict": False},
        "delete_repeats": -1,
+        "delete_repeats_channels": {},


This should be part of the channel scope, not a dict in the guild settings.

Flame442 · 2022-03-18T22:14:36Z

redbot/cogs/mod/settings.py

+        self, ctx: commands.Context, repeats: int = None, for_channel: str = None
+    ):
        """Enable auto-deletion of repeated messages.

        Must be between 2 and 20.

        Set to -1 to disable this feature.
+
+        Add "this" if you want to specify for only one channel


The argument to specify the setting should only apply for a channel should be an optional discord.TextChannel (or perhaps a greedy one if we want to support passing multiple channels?). It should not be a string, and it should not lock the user into only picking the current channel.

Flame442 · 2022-03-18T22:14:49Z

redbot/cogs/mod/settings.py

+            guild_cache = self.cache[guild.id] = await create_new_cache(self.config, guild)
+
+        delete_repeats_channels = await self.config.guild(guild).delete_repeats_channels.all()
+        print(delete_repeats_channels)


See above comment about print.

Flame442 · 2022-03-18T22:15:10Z

redbot/cogs/mod/utils.py

+
+    # Create already keys for custom amount of repeated messages per channel
+    repeats_channels = await config.guild(guild).delete_repeats_channels.all()
+    print(repeats_channels)


See above comment about print.

Flame442

This PR is looking a lot better than the first time I reviewed it. Still has quite a few things that need to be revised, but I can see it coming together now.

Flame442 · 2022-11-28T21:51:21Z

redbot/cogs/mod/settings.py

    @modset.command()
    @commands.guild_only()
-    async def deleterepeats(self, ctx: commands.Context, repeats: int = None):
+    async def deleterepeats(self, ctx: commands.Context, repeats: int = None, *channels):


Suggested change

async def deleterepeats(self, ctx: commands.Context, repeats: int = None, *channels):

async def deleterepeats(self, ctx: commands.Context, repeats: int = None, *channels: discord.TextChannel):

As I suggested in my previous review, it is far easier to just type hint the channels argument so dpy automatically converts it than to attempt to process it after the fact.

Flame442 · 2022-11-28T21:55:31Z

redbot/cogs/mod/settings.py

+        Add channels if you want to custom the value for the channels (#name_of_channel)
+
+        Set to 0 if you want to use the default value for the channel selected


Suggested change

Add channels if you want to custom the value for the channels (#name_of_channel)

Set to 0 if you want to use the default value for the channel selected

Channel specific overrides can be created by using this command with a passed in value for `channels`.

If one or more channels are provided, `repeats` can be set to 0 to make those channels use the server default.

I don't think this is perfect, but it's at least a bit more descriptive than what is currently there.

Flame442 · 2022-11-28T21:55:57Z

redbot/cogs/mod/settings.py

+        Set to 0 if you want to use the default value for the channel selected
+
        """
+        test = 1


Suggested change

test = 1

This line should not be here, I assume this is left over from your local debugging.

Flame442 · 2022-11-28T21:56:29Z

redbot/cogs/mod/settings.py

+        custom = ""
+        channel_data = await self.config.all_channels()
+        for channel in ctx.guild.channels:
+            data = data = channel_data.get(channel.id, None)


Suggested change

data = data = channel_data.get(channel.id, None)

data = channel_data.get(channel.id, None)

If you aren't setting multiple different variables, there is no reason to use this syntax.

Flame442 · 2022-11-28T21:58:36Z

redbot/cogs/mod/settings.py

        )
+        custom = ""
+        channel_data = await self.config.all_channels()
+        for channel in ctx.guild.channels:


Since this string now has a non-determinant length due to this loop, the eventual send of this command should be modified to ensure the resulting message is under 2000 characters, and to properly pagify the message if it is not.

Flame442 · 2022-11-28T22:05:29Z

redbot/cogs/mod/utils.py

+def lambda_cache(repeats_channel):
+    return lambda: deque(maxlen=repeats_channel if repeats_channel != -1 else 0)
+
+
+async def create_new_cache(config: Config, guild: discord.Guild):
+    repeats = await config.guild(guild).delete_repeats()
+    channel_data = await config.all_channels()
+    guild_cache = defaultdict(
+        lambda: defaultdict(lambda: deque(maxlen=repeats if repeats != -1 else 0))
+    )
+    # Create already keys for custom amount of repeated messages per channel
+    for channel in guild.channels:
+        data = channel_data.get(channel.id, None)
+        if data is not None and data["delete_repeats"]:
+            guild_cache[channel.id] = defaultdict(lambda_cache(data["delete_repeats"]))
+    return guild_cache


I'm not certain, but this file might be part of the public API, and if so these functions should either not be in this file or be private.

Flame442 · 2022-11-28T22:25:47Z

redbot/cogs/mod/settings.py

+        fails = set()
+        success = set()
+        for channel_id in channels:
+            # Removing every non-digit number
+            channel_id_digits = "".join(ch for ch in channel_id if ch.isdigit())
+            channel_id_digits
+            channel = ctx.guild.get_channel(
+                int(channel_id_digits) if len(channel_id_digits) != 0 else 0
+            )
+            if channel is None:
+                fails.add(channel_id)
+            else:
+                success.add(channel)
+
+        if len(fails) > 0:
+            msg = _("Discord failed to recognize the following channels : (")
+            for channel in fails:
+                msg += _(f"{channel},")
+            await ctx.send(msg + ")")


This entire block is not needed if you use the converter,

Flame442 · 2022-11-28T22:36:55Z

redbot/cogs/mod/settings.py

+                    channels_string = ""
+                    for channel in success:
+                        channels_string += _("{name}, ").format(name=channel.name)
+                        await self.config.channel(channel).delete_repeats.set(repeats)
+                    msg = _("Repeated messages will be ignored for the following channels : ( ")
+
+                    await ctx.send(_(msg + channels_string + ")"))
+                    self.cache[guild.id][channel.id] = defaultdict(lambda: deque(maxlen=0))


Suggested change

channels_string = ""

for channel in success:

channels_string += _("{name}, ").format(name=channel.name)

await self.config.channel(channel).delete_repeats.set(repeats)

msg = _("Repeated messages will be ignored for the following channels : ( ")

await ctx.send(_(msg + channels_string + ")"))

self.cache[guild.id][channel.id] = defaultdict(lambda: deque(maxlen=0))

channels_string = ", ".join(channel.name for channel in success)

for channel in success:

await self.config.channel(channel).delete_repeats.set(repeats)

self.cache[guild.id][channel.id] = defaultdict(lambda: deque(maxlen=0))

msg = _("Repeated messages will be ignored for the following channels: {channels_string}").format(channels_string=channels_string)

await ctx.send(msg)

You were only updating the cache for the last guild currently, since it was setting outside the loop.

Translation strings (_("Text to translate here")) should only be used on hardcoded strings or strings with .format applied outside of the _(), not on strings that are concatenated from multiple variables, and can thus be dynamic at runtime (IE _(msg + channels_string + ")") should not be a translated string).

Note that I kept success as the variable to loop over, this variable will need to be changed to channels in all places you're using it once you change the parameter to convert to channels automatically. I just don't want to add suggestions to every usage of it, since there's quite a few :P

Flame442 · 2022-11-28T22:39:59Z

redbot/cogs/mod/settings.py

+                    for channel in success:
+                        await self.config.channel(channel).delete_repeats.set(repeats)
+                        # reset the cache for this channel
+                        self.cache[guild.id].pop(channel.id, None)


You don't have any output in this branch.

Flame442 · 2022-11-28T22:42:40Z

redbot/cogs/mod/settings.py

+                    channels_string = ""
+                    for channel in success:
+                        channels_string += _("{name}, ").format(name=channel.name)
+                        await self.config.channel(channel).delete_repeats.set(repeats)
+                        self.cache[guild.id][channel.id] = defaultdict(
+                            lambda: deque(maxlen=repeats)
+                        )
+                    msg = _(
+                        "Messages repeated up to {num} times will be deleted for the following channels : ( "
+                    ).format(num=repeats)
+                    await ctx.send(_(msg + channels_string + ")"))


Suggested change

channels_string = ""

for channel in success:

channels_string += _("{name}, ").format(name=channel.name)

await self.config.channel(channel).delete_repeats.set(repeats)

self.cache[guild.id][channel.id] = defaultdict(

lambda: deque(maxlen=repeats)

)

msg = _(

"Messages repeated up to {num} times will be deleted for the following channels : ( "

).format(num=repeats)

await ctx.send(_(msg + channels_string + ")"))

channels_string = ", ".join(channel.name for channel in success)

for channel in success:

await self.config.channel(channel).delete_repeats.set(repeats)

self.cache[guild.id][channel.id] = defaultdict(

lambda: deque(maxlen=repeats)

)

msg = _(

"Messages repeated up to {num} times will be deleted for the following channels: {channels_string}"

).format(num=repeats, channels_string=channels_string)

await ctx.send(msg)

YannPollet added 2 commits March 4, 2022 17:14

feat: custom deleterepeats for channels

b7b58db

Spell : removed some features added to reinit the variables

fe92cd8

github-actions bot added the Category: Cogs - Mod This is related to the Mod cog. label Mar 10, 2022

Flame442 requested changes Mar 18, 2022

View reviewed changes

YannPollet and others added 2 commits April 1, 2022 20:47

Revisions resolved

92bca44

Merge branch 'V3/develop' into V3/develop

171546f

Jackenmen added Blocked and removed Blocked labels Apr 2, 2022

Merge branch 'V3/develop' into V3/develop

f8fcdbd

Flame442 self-assigned this Aug 29, 2022

Merge branch 'V3/develop' into V3/develop

73be62e

Flame442 removed the QA: Changes Requested Used by few QA members. Awaiting changes requested by maintainers or QA. label Sep 3, 2022

Flame442 requested changes Nov 28, 2022

View reviewed changes

Flame442 added the QA: Changes Requested Used by few QA members. Awaiting changes requested by maintainers or QA. label Nov 28, 2022

Merge branch 'V3/develop' into V3/develop

fbae041

		repeats = await self.config.guild(guild).delete_repeats()
		repeats_channels = await self.config.guild(guild).delete_repeats_channels.all()

		print(guild_cache)
		print("-----------------------------------------------------------------------")

	guild_cache = (
	defaultdict(lambda: defaultdict(lambda: deque(maxlen=repeats)))
	if repeats != -1
	else dict()
	)

	guild_cache = self.cache.get(guild.id, None)
	if guild_cache is None:
	repeats = await self.config.guild(guild).delete_repeats()
	if repeats == -1:
	return False
	guild_cache = self.cache[guild.id] = defaultdict(lambda: deque(maxlen=repeats))

	async def deleterepeats(self, ctx: commands.Context, repeats: int = None, *channels):
	async def deleterepeats(self, ctx: commands.Context, repeats: int = None, *channels: discord.TextChannel):

		Add channels if you want to custom the value for the channels (#name_of_channel)

		Set to 0 if you want to use the default value for the channel selected

	data = data = channel_data.get(channel.id, None)
	data = channel_data.get(channel.id, None)

Uh oh!

Making DeleteRepeats having a custom value per channel #5607

Are you sure you want to change the base?

Making DeleteRepeats having a custom value per channel #5607

Uh oh!

Conversation

YannPollet commented Mar 10, 2022

Description of the changes

Have the changes in this PR been tested?

Uh oh!

Flame442 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flame442 Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flame442 Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flame442 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Flame442 Mar 23, 2022 •

edited

Loading

Flame442 Mar 23, 2022 •

edited

Loading