Skip to content

Clean improvements#1989

Merged
ChrisLovering merged 7 commits into
mainfrom
clean_improvements
Dec 16, 2021
Merged

Clean improvements#1989
ChrisLovering merged 7 commits into
mainfrom
clean_improvements

Conversation

@mbaruh
Copy link
Copy Markdown
Member

@mbaruh mbaruh commented Dec 2, 2021

This PR follows observations of how the new clean features were being used.

Changes:

  • All clean commands now use the clean limit (message, time delta, ISO datetime) instead of traverse.
  • Consequently, clean all has been removed as clean until now effectively fulfills that role.
  • Removes the cache usage argument from the clean commands. The cache will be used if the age of the oldest message requested for cleaning is younger than the oldest message in the cache (meaning, all of the messages we're looking for are cached).
  • Along with a small speed improvement in the implementation, when specifying "*" for the channels the command will now delete only from public channels for further speed-up.

Fixes:

  • Iterating over the cache was done from the wrong end.
  • The file was using the old logger due to a faulty merge.

All clean commands now use the clean limit (message, time delta, ISO datetime) instead of `traverse`.

Consequently, `clean all` has been removed as `clean until` now effectively fulfills that role.
Removes the cache usage argument from the clean commands.
Cache usage is now an implementation detail.
The cache will be used if the age of the oldest message requested for cleaning is younger than the oldest message in the cache.

Additionally fixes the logger to the one used in the rest of the bot (caused by a faulty merge).
@mbaruh mbaruh requested a review from ChrisLovering December 2, 2021 20:39
@mbaruh mbaruh added a: moderation Related to community moderation functionality: (moderation, defcon, verification) p: 2 - normal Normal Priority t: bug Something isn't working t: enhancement Changes or improvements to existing features labels Dec 2, 2021
Copy link
Copy Markdown
Member

@ChrisLovering ChrisLovering left a comment

Choose a reason for hiding this comment

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

Some very nice code Zig 👍 I really like the change you made to the cache, should make this much nicer to use :D

Comment thread bot/exts/moderation/clean.py
Copy link
Copy Markdown
Contributor

@Shivansh-007 Shivansh-007 left a comment

Choose a reason for hiding this comment

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

Very cool 👍🏻

@mbaruh mbaruh added the review: do not merge The PR can be reviewed but cannot be merged now label Dec 5, 2021
@mbaruh
Copy link
Copy Markdown
Member Author

mbaruh commented Dec 5, 2021

Just noticed a bug this PR is going to introduce, will fix soon.

@mbaruh mbaruh marked this pull request as draft December 5, 2021 16:29
Previously the cache was only used to delete from all channels. I didn't add a channels check when I changed it.
When specifying all channels, the command now skips private channels to optimize for speed.
@mbaruh mbaruh marked this pull request as ready for review December 5, 2021 22:12
@mbaruh mbaruh removed the review: do not merge The PR can be reviewed but cannot be merged now label Dec 5, 2021
@mbaruh mbaruh requested a review from ChrisLovering December 5, 2021 22:12
Copy link
Copy Markdown
Member

@ChrisLovering ChrisLovering left a comment

Choose a reason for hiding this comment

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

Tested latest changes 👍

channels = {
channel for channel in ctx.guild.channels
if isinstance(channel, TextChannel)
# Assume that non-public channels are not needed to optimize for speed.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand why it's done, but what happens if we have "public" channels that are still locked behind a role such as during codejams.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The participants channel stays public, just not writeable. If we get to the point of cleaning a user from the team leaders channel, I think a more focused action will be made anyway, likely including the disqualification of the participant. Specifying such a channel explicitly in that case seems like an acceptable edge case. And if it stops being acceptable we can consider adding another shortcut for actually-all channels.

Copy link
Copy Markdown
Contributor

@Bluenix2 Bluenix2 left a comment

Choose a reason for hiding this comment

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

From my very limited experience with the clean cog... it works! Great thank you my favourite peppa-pig look-alike. Nice work here
high5

@ChrisLovering ChrisLovering merged commit 67412f8 into main Dec 16, 2021
@ChrisLovering ChrisLovering deleted the clean_improvements branch December 16, 2021 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: moderation Related to community moderation functionality: (moderation, defcon, verification) p: 2 - normal Normal Priority t: bug Something isn't working t: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants