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

breaking: Updated dpp::role comparison operators to now consider role IDs #1333

Merged
merged 2 commits into from
Nov 16, 2024

Conversation

Commandserver
Copy link
Member

@Commandserver Commandserver commented Nov 12, 2024

This is a breaking change for anyone using the comparison operators of the dpp::role class!

If two roles have the same position, their IDs are compared to find out which role is higher in the hierarchy. The higher the IDs are, the lower they are. Previously, only the position was compared.

Documentation: https://discord.com/developers/docs/topics/permissions#role-object-role-structure

Code change checklist

  • I have ensured that all methods and functions are fully documented using doxygen style comments.
  • My code follows the coding style guide.
  • I tested that my change works before raising the PR.
  • I have ensured that I did not break any existing API calls.
  • I have not built my pull request using AI, a static analysis tool or similar without any human oversight.

…lid_argument when roles of two different guilds
@Commandserver Commandserver added documentation Improvements or additions to documentation code Improvements or additions to code. labels Nov 12, 2024
Copy link

netlify bot commented Nov 12, 2024

Deploy Preview for dpp-dev ready!

Name Link
🔨 Latest commit 5e557b0
🔍 Latest deploy log https://app.netlify.com/sites/dpp-dev/deploys/67348d57e0a0160008bade82
😎 Deploy Preview https://deploy-preview-1333--dpp-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

include/dpp/role.h Show resolved Hide resolved
@braindigitalis
Copy link
Contributor

where in the docs is this behaviour of identical numeric values? as you can't set these values via the client, how could this happen in the first place? either way this is a breaking change that should be labelled as such imho

@Commandserver Commandserver changed the title fix: Updated dpp::role comparison operators breaking: Updated dpp::role comparison operators Nov 12, 2024
@Commandserver
Copy link
Member Author

where in the docs is this behaviour of identical numeric values?

it's not documented.

as you can't set these values via the client, how could this happen in the first place?

The only thing i know about is when you add bots to the server. Their roles always have a position of 1.

@braindigitalis
Copy link
Contributor

where in the docs is this behaviour of identical numeric values?

it's not documented.

as you can't set these values via the client, how could this happen in the first place?

The only thing i know about is when you add bots to the server. Their roles always have a position of 1.

im curious what discord's own opinion on this undocumented behaviour is, and if it should be supported, documented, or fixed on their end. I'll ask before we merge this in (or not!) and then we can proceed :)

@braindigitalis
Copy link
Contributor

braindigitalis commented Nov 12, 2024

seems it is documented: https://discord.com/developers/docs/topics/permissions#role-object-role-structure
See the field for "position"

position of this role (roles with the same position are sorted by id)

Lets get this merged in then, but first:

As such, the == operator isn't particularly relevant here but should be preserved as it is used for sorting, finding and hashing if you store roles in a set etc, you may want to compare the role against a copy of that same role. As such we should fix == to work properly (returns true if id and position are the same in both objects)

This also means less than cant just be "not greater than"

@Commandserver
Copy link
Member Author

As such, the == operator isn't particularly relevant here but should be preserved as it is used for sorting, finding and hashing if you store roles in a set etc, you may want to compare the role against a copy of that same role. As such we should fix == to work properly (returns true if id and position are the same in both objects)

wouldn't it be enough to just compare the IDs, since a role is already unique by its ID in a set of roles?

@braindigitalis
Copy link
Contributor

As such, the == operator isn't particularly relevant here but should be preserved as it is used for sorting, finding and hashing if you store roles in a set etc, you may want to compare the role against a copy of that same role. As such we should fix == to work properly (returns true if id and position are the same in both objects)

wouldn't it be enough to just compare the IDs, since a role is already unique by its ID in a set of roles?

sounds reasonable to me

@Commandserver Commandserver changed the title breaking: Updated dpp::role comparison operators breaking: Updated dpp::role comparison operators to now consider role IDs Nov 13, 2024
@Commandserver Commandserver merged commit 561cc21 into brainboxdotcc:dev Nov 16, 2024
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Improvements or additions to code. documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants