Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support bulk banning in guilds #2421

Merged
merged 19 commits into from
Apr 26, 2024

Conversation

NeloBlivion
Copy link
Member

@NeloBlivion NeloBlivion commented Apr 13, 2024

Summary

Guild.ban now accepts up to 200 members
Currently, this returns two lists:

successes, failures = await guild.ban(*members, reason="Hacked")

Alternatively, we could have a new GuildBulkBan object with attributes banned and failed

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

Copy link
Member

@plun1331 plun1331 left a comment

Choose a reason for hiding this comment

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

Possibly make a new method instead (guild.bulk_ban) since they're different endpoints, and similar stuff is split as well (e.g. fetch_member vs fetch_members)

discord/guild.py Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
discord/guild.py Outdated Show resolved Hide resolved
discord/guild.py Show resolved Hide resolved
discord/guild.py Outdated Show resolved Hide resolved
discord/guild.py Outdated Show resolved Hide resolved
@plun1331 plun1331 added priority: low Low Priority status: awaiting review Awaiting review from a maintainer feature Implements a feature labels Apr 13, 2024
@plun1331 plun1331 added this to the v2.6 milestone Apr 13, 2024
NeloBlivion and others added 3 commits April 13, 2024 18:40
Co-authored-by: plun1331 <plun1331@gmail.com>
Signed-off-by: UK <41271523+NeloBlivion@users.noreply.github.com>
@NeloBlivion
Copy link
Member Author

Possibly make a new method instead (guild.bulk_ban) since they're different endpoints, and similar stuff is split as well (e.g. fetch_member vs fetch_members)

I did consider this initially, but after mulling it over I figured it made more sense to compare to Member.add_roles/remove_roles, which similarly accepts 1 or multiple Snowflakes and incorporates different routes depending on the situation

@plun1331
Copy link
Member

Possibly make a new method instead (guild.bulk_ban) since they're different endpoints, and similar stuff is split as well (e.g. fetch_member vs fetch_members)

I did consider this initially, but after mulling it over I figured it made more sense to compare to Member.add_roles/remove_roles, which similarly accepts 1 or multiple Snowflakes and incorporates different routes depending on the situation

My main issue is we end up having multiple different return types, which isn't a massive problem but might be confusing as some people might pass a list and not expect it to give None back (if the list had a length of 1), similar to some confusion that was discussed a while ago about the return types of ApplicationContext.respond

@Dorukyum
Copy link
Member

I agree with plun, a new method should suffice.

@Lulalaby
Copy link
Member

Upon implementing it myself I agree with doru, a seperate method is needed

Copy link
Member

@Lulalaby Lulalaby left a comment

Choose a reason for hiding this comment

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

Revert changes and introduce bulk banning as new method bulk_ban.
Also we should finally remove delete message days

@NeloBlivion
Copy link
Member Author

With this, 2.6 is officially breaking so please include the relevant warnings in any announcements

@plun1331 plun1331 requested a review from Lulalaby April 22, 2024 13:16
@plun1331 plun1331 enabled auto-merge (squash) April 22, 2024 13:17
@Lulalaby Lulalaby disabled auto-merge April 22, 2024 13:41
@Lulalaby Lulalaby requested a review from Dorukyum April 22, 2024 13:41
@Lulalaby
Copy link
Member

Before we merge it, @Dorukyum, do you agree on removing the obsolete parameter completely in this PR?

@Dorukyum
Copy link
Member

Before we merge it, @Dorukyum, do you agree on removing the obsolete parameter completely in this PR?

It's fine. A seperate pr for each change would obviously be better, but the changes here are relevant enough I guess.

@plun1331 plun1331 enabled auto-merge (squash) April 26, 2024 14:08
@Lulalaby Lulalaby disabled auto-merge April 26, 2024 15:03
@Lulalaby Lulalaby merged commit a1439ba into Pycord-Development:master Apr 26, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Implements a feature priority: low Low Priority status: awaiting review Awaiting review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants