Skip to content

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)

@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
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