Skip to content

Conversation

SimonVerhoeven
Copy link
Contributor

Updates the delete history options as follows:
recent => day
all => week

To make how much is cleared clearer.

Resolves #1081

@SimonVerhoeven SimonVerhoeven requested a review from a team as a code owner April 12, 2024 16:33
@CLAassistant
Copy link

CLAassistant commented Apr 12, 2024

CLA assistant check
All committers have signed the CLA.

@SquidXTV
Copy link
Member

I would say the description needs to be updated as well:

the amount of days of the message history to delete, none means no messages are deleted.

if the options are none/day/week

@SimonVerhoeven
Copy link
Contributor Author

Would the portion of the message history to delete. suffice?

@SquidXTV
Copy link
Member

Would the portion of the message history to delete. suffice?

Well wouldn't it be even fine to just simplify it to the message history to delete. and then the options are self-explanatory?
Maybe better if the options are named like last day/last week as well.

SquidXTV
SquidXTV previously approved these changes Apr 17, 2024
Copy link
Member

@SquidXTV SquidXTV left a comment

Choose a reason for hiding this comment

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

<3

@tj-wazei
Copy link

tj-wazei commented May 6, 2024

I approved for the renaming of things but the ban command in it's logic looks incorrect to me (out of scope for Simon).

The Bulk Message Delete API https://discord.com/developers/docs/resources/channel#delete-message

You can delete messages that are younger than 2 weeks. The API can only delete a maximum of 100 messages. Which means for us to do a whole weeks worth of messages, we'd have to make multiple API calls in batches of 100.

Something to look into. Maybe create a new Issue for investigation?

@Zabuzard
Copy link
Member

Zabuzard commented May 6, 2024

i dont mind. so far it wasnt a problem. afaik these choices are passed directly as they are to the API and align with the options u have when u ban through the native discord UI without our command. and thats fine.
adjust the names if it helps, but there isnt really much need for the actions to do anything beyond that

@tj-wazei tj-wazei merged commit b097294 into Together-Java:develop May 6, 2024
@ankitsmt211 ankitsmt211 mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ban - the delete history options are vague

5 participants