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

fix(Guild): sort roles with the same position in the correct order #3184

Merged
merged 1 commit into from Apr 8, 2019

Conversation

Projects
None yet
2 participants
@SpaceEEC
Copy link
Member

commented Apr 5, 2019

Please describe the changes this PR makes and why it should be merged:

Roles are currently sorted in the incorrect order.

For example once created the roles will appear in the role manager as the following:

  • old role, position 2
  • new role1, position 1
  • new role2, position 1
  • new role3, position 1
  • @everyone, position 0

They would however be incorrectly sorted as:

  • old role, calculatedPosition 4
  • new role3, calculatedPosition 3
  • new role2, calculatedPosition 2
  • new role1, calculatedPosition 1
  • @everyone, calculatedPosition 0

With this pr they are sorted correctly:

  • old role, calculatedPosition 4
  • new role1, calculatedPosition 3
  • new role2, calculatedPosition 2
  • new role3, calculatedPosition 1
  • @everyone, calculatedPosition 0

For reference the code I used to create the roles:

const guild = message.guild;

const roles = [];
for (const roleName of ['new role1', 'new role2', 'new role3']) {
	roles.push(await guild.createRole({ name: roleName }));
}

console.log(roles.map(role => [role.name, role.position, role.calculatedPosition]));

The code responsible for sorting the roles is doing a.id - b.id while it should be b.id - a.id:

Long.fromString(a.id).sub(Long.fromString(b.id)).toNumber()

Role.comparePositions however does it "correctly":

if (role1.position === role2.position) return role2.id - role1.id;

On master this seems to be fixed already:

discord.js/src/util/Util.js

Lines 305 to 306 in bfab203

parseInt(b.id.slice(0, -10)) - parseInt(a.id.slice(0, -10)) ||
parseInt(b.id.slice(10)) - parseInt(a.id.slice(10))

Closes #3175

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@SpaceEEC SpaceEEC merged commit 923c945 into discordjs:11.4-dev Apr 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@SpaceEEC SpaceEEC deleted the SpaceEEC:fix/role_sort branch Apr 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.