Skip to content

Clean improvments#2189

Merged
mbaruh merged 6 commits into
mainfrom
clean_improv
Jun 12, 2022
Merged

Clean improvments#2189
mbaruh merged 6 commits into
mainfrom
clean_improv

Conversation

@mbaruh
Copy link
Copy Markdown
Member

@mbaruh mbaruh commented Jun 11, 2022

  • Fixed a bug where clean between deleted the last message. Clarified in the appropriate command descriptions that the clean limits are exclusive.
  • Added a shortcut to a common use case of deleting all recent public messages of a user. Since the purge alias was never used for the clean group I repurposed it for this new command.

mbaruh added 2 commits June 11, 2022 19:02
The command cleans all public messages from a user. This is a private case that is common enough that it probably deserves a shortcut.

Since the `purge` alias is never used for the clean group I repurposed it for this new command.
@mbaruh mbaruh requested review from Den4200, jb3 and ks129 as code owners June 11, 2022 16:36
@mbaruh mbaruh added t: bug Something isn't working t: feature New feature or request a: moderation Related to community moderation functionality: (moderation, defcon, verification) p: 2 - normal Normal Priority labels Jun 11, 2022
Comment thread bot/exts/moderation/clean.py Outdated
Comment thread bot/exts/moderation/clean.py
def predicate_range(message: Message) -> bool:
"""Check if the message age is between the two limits."""
return first_limit <= message.created_at <= second_limit
return first_limit < message.created_at < second_limit
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'm confused how this makes a difference. The messages are fetched using Messageable.history() with the same limits. For history(), those limits are already exclusive. I don't understand why predicate_range and predicate_after are even needed.

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.

Because the search might go through the cache. If history is indeed exclusive then I guess this predicate could be dropped in those cases, but I don't mind another guarantee if it already exists.

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.

predicate_after should also be exclusive then.

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.

Hmm good point. It doesn't cause a bug because the iteration excludes the stopping point.

Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

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

Comment thread bot/exts/moderation/clean.py Outdated
@mbaruh
Copy link
Copy Markdown
Member Author

mbaruh commented Jun 12, 2022

This argument can be None, so it should be annotated as such https://github.com/python-discord/bot/blob/0bbcde6af81405380bbae1eca3556338331b03fa/bot/exts/moderation/clean.py#L244=

There is an issue with the annotation, but it's actually different. after is the one which isn't optional, while before is. This is enforced by _validate_input

@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Jun 12, 2022

This argument can be None, so it should be annotated as such https://github.com/python-discord/bot/blob/0bbcde6af81405380bbae1eca3556338331b03fa/bot/exts/moderation/clean.py#L244=

There is an issue with the annotation, but it's actually different. after is the one which isn't optional, while before is. This is enforced by _validate_input

Technically both are optional since they're just passed to history(), and that function allows both to be None.

@mbaruh
Copy link
Copy Markdown
Member Author

mbaruh commented Jun 12, 2022

Technically both are optional since they're just passed to history(), and that function allows both to be None.

yeah but this is about how the function is expected to be used, rather than what it can technically accept

@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Jun 12, 2022

Technically both are optional since they're just passed to history(), and that function allows both to be None.

yeah but this is about how the function is expected to be used, rather than what it can technically accept

I don't feel strongly about this but if it were me I wouldn't impose artificial constraints on it.

@mbaruh
Copy link
Copy Markdown
Member Author

mbaruh commented Jun 12, 2022

If I write a function which takes a number and adds 1 to it, I could assign a default value of 0 to the number, but I have no reason to. That's not how I want the function to behave. If I don't supply it a number I want it to yell at me, not because of what the function can do in practice if I allow it, but by my choice.

mbaruh and others added 2 commits June 13, 2022 01:28
@mbaruh mbaruh merged commit 4590dfb into main Jun 12, 2022
@mbaruh mbaruh deleted the clean_improv branch June 12, 2022 23:10
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: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants