feat: Support bulk banning in guilds

Pycord-Development / pycord

Pycord is a modern, easy to use, feature-rich, and async ready API wrapper for Discord written in Python

https://docs.pycord.dev

MIT License

2.7k stars 458 forks source link

feat: Support bulk banning in guilds #2421

Closed NeloBlivion closed 4 months ago

NeloBlivion commented 5 months ago

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.
[x] This PR adds something new (e.g. new method or parameters).
[x] 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

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

NeloBlivion commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

I agree with plun, a new method should suffice.

Lulalaby commented 4 months ago

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

NeloBlivion commented 4 months ago

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

Lulalaby commented 4 months ago

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

Dorukyum commented 4 months ago

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.