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

Add support for Discord.py 1.7+ #534

Merged
merged 1 commit into from
Jun 21, 2021
Merged

Conversation

ChrisLovering
Copy link
Member

@ChrisLovering ChrisLovering commented Jun 18, 2021

With the addition of new permissions in Discord, the max permission number now exceeds that which can be stored in an int.

This needs to be merged before we can bump the bot up to 1.7 and Discord.py add support for these new permissions. I have removed the max value validator, as this just adds maintenance burden for us each time Discord adds new permissions.

See this table in the Discord API docs to see that the new max number 1 << 36
https://discord.com/developers/docs/topics/permissions#permissions-bitwise-permission-flags

@coveralls
Copy link

coveralls commented Jun 18, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling c40ed8c on add-support-for-d.py-1.7 into 297337f on main.

ToxicKidz
ToxicKidz previously approved these changes Jun 18, 2021
@Numerlor
Copy link
Contributor

Is the MaxValueValidator necessary? It doesn't feel like it adds much value while adding a maintenance cost. The current max value in discord.py is also 33 bit so I'm not even sure if that or 36 would fit more

@vcokltfre
Copy link
Contributor

Bear in mind thread permissions are coming at some point, so with a constraint on bit length another PR like this will need to be made to upgrade to a d.py version that supports those permissions

@ChrisLovering
Copy link
Member Author

Is the MaxValueValidator necessary? It doesn't feel like it adds much value while adding a maintenance cost. The current max value in discord.py is also 33 bit so I'm not even sure if that or 36 would fit more

Hmm yea, if we went with d.py, then we'd need to bump this with a migration each time. If we went with 36, it gives us some time, but it won't be long until Discord adds new permissions.

I tend to agree that we could remove this validator entirely.

vcokltfre
vcokltfre previously approved these changes Jun 18, 2021
Copy link
Contributor

@vcokltfre vcokltfre left a comment

Choose a reason for hiding this comment

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

Approving, but bear in mind above comments for the future

@Numerlor
Copy link
Contributor

Bear in mind thread permissions are coming at some point, so with a constraint on bit length another PR like this will need to be made to upgrade to a d.py version that supports those permissions

the 36 bit val should take in account the new thread perms (unless some new ones are added after), but then it won't be a correct check for current d.py. I'd say the validator should just be removed

@ChrisLovering ChrisLovering marked this pull request as draft June 18, 2021 20:18
@ChrisLovering
Copy link
Member Author

converted to draft as I am going to remove the max value validator later this evening.

BigInt is needed as Discord's permissions number now exceeds that which can be stored
in a normal int. I have removed the max value validator, as this just adds
maintanence burden for us each time Discord adds new permissions.
@ChrisLovering ChrisLovering marked this pull request as ready for review June 18, 2021 21:17
@ChrisLovering ChrisLovering dismissed stale reviews from vcokltfre and ToxicKidz June 18, 2021 21:17

Changed since approval

@ChrisLovering ChrisLovering merged commit 7bf685d into main Jun 21, 2021
@ChrisLovering ChrisLovering deleted the add-support-for-d.py-1.7 branch June 21, 2021 15:56
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.

None yet

7 participants