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

Proposed Solution for #674 Permissions Changes #743

Merged
merged 9 commits into from Oct 26, 2017

Conversation

@Chris-Johnston
Copy link
Contributor

commented Jul 8, 2017

Changed the ChannelPermission, GuildPermission enums to match the Discord API. Adjusted relevant methods to match this change. Also added view audit logs permission to guild permission while I was at it.

Will using System.Math.Pow cause compatibility issues? (had to import System)

This is a breaking change, both the values and names of the enum definition have been modified.

My discord handle is ChrisJ#8703 if you want to contact me.

Initial commit of changes. Changed permissions from bitwise index to …
…use bitwise flags instead. Modified relevant methods involved
@@ -1,40 +1,29 @@
namespace Discord
{
public enum ChannelPermission : byte
public enum ChannelPermission : ulong

This comment has been minimized.

Copy link
@foxbot

foxbot Jul 8, 2017

Member

Permissions are bitflags; this enum should be marked as a flags enum.

This comment has been minimized.

Copy link
@Chris-Johnston
//Administrator = 3,
ManageChannel = 4,
//ManageGuild = 5,
CREATE_INSTANT_INVITE = 0x00000001,

This comment has been minimized.

Copy link
@foxbot

foxbot Jul 8, 2017

Member

While enumeration values are constants, proper style should be pascal case.

This comment has been minimized.

Copy link
@Joe4evr

Joe4evr Jul 8, 2017

Contributor

Also, the comments categorizing each section of permissions would be nice to still have in source. And maybe put an _ every two digits for readability.

@@ -1,40 +1,34 @@
namespace Discord
{
public enum GuildPermission : byte
public enum GuildPermission : ulong

This comment has been minimized.

Copy link
@foxbot

foxbot Jul 8, 2017

Member

See above

@Joe4evr

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2017

You updated just before I commented.

Also, the comments categorizing each section of permissions would be nice to still have in source. And maybe put an _ every two digits for readability.

@Chris-Johnston

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2017

Added @Joe4evr suggested changes

@Chris-Johnston

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2017

Added @foxbot 's suggested changes

@@ -73,7 +73,7 @@ public static ChannelPermissions All(IChannel channel)
public bool UseVAD => Permissions.GetValue(RawValue, ChannelPermission.UseVAD);

/// <summary> If True, a user may adjust permissions. This also implictly grants all other permissions. </summary>
public bool ManagePermissions => Permissions.GetValue(RawValue, ChannelPermission.ManagePermissions);
public bool ManagePermissions => Permissions.GetValue(RawValue, ChannelPermission.ManageRoles);

This comment has been minimized.

Copy link
@SubZero0

SubZero0 Jul 8, 2017

Contributor

Should probably rename "ManagePermissions" to "ManageRoles" for consistency

This comment has been minimized.

Copy link
@Chris-Johnston

Chris-Johnston Jul 8, 2017

Author Contributor

Added in 236775c and 9f1c6d0

@@ -59,7 +60,7 @@ public static OverwritePermissions DenyAll(IChannel channel)
public PermValue UseVAD => Permissions.GetValue(AllowValue, DenyValue, ChannelPermission.UseVAD);

/// <summary> If Allowed, a user may adjust permissions. This also implictly grants all other permissions. </summary>
public PermValue ManagePermissions => Permissions.GetValue(AllowValue, DenyValue, ChannelPermission.ManagePermissions);
public PermValue ManagePermissions => Permissions.GetValue(AllowValue, DenyValue, ChannelPermission.ManageRoles);

This comment has been minimized.

Copy link
@SubZero0

SubZero0 Jul 8, 2017

Contributor

See above

Chris-Johnston added some commits Jul 8, 2017

@FiniteReality
Copy link
Collaborator

left a comment

Looks fairly good, though I think we could do some more clean up afterwards

{
if ((RawValue & x) != 0)
perms.Add((ChannelPermission)i);
ulong flag = (ulong)Math.Pow(2, i);

This comment has been minimized.

Copy link
@FiniteReality

FiniteReality Jul 8, 2017

Collaborator

Use 1<<i here rather than Math.Pow

{
if ((RawValue & x) != 0)
perms.Add((GuildPermission)i);
ulong flag = (ulong)Math.Pow(2, i);

This comment has been minimized.

Copy link
@FiniteReality

FiniteReality Jul 8, 2017

Collaborator

Likewise, 1<<i rather than Math.Pow

{
if ((AllowValue & x) != 0)
perms.Add((ChannelPermission)i);
ulong flag = (ulong)Math.Pow(2, i);

This comment has been minimized.

Copy link
@FiniteReality

FiniteReality Jul 8, 2017

Collaborator

1<<i

{
if ((DenyValue & x) != 0)
perms.Add((ChannelPermission)i);
ulong flag = (ulong)Math.Pow(2, i);

This comment has been minimized.

Copy link
@FiniteReality

FiniteReality Jul 8, 2017

Collaborator

1<<i

@Chris-Johnston

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2017

Added @FiniteReality 's requested changes

@Chris-Johnston

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2017

Formatted the two enums so that they are displayed in neat columns in the source.

Also, could we add the Hacktoberfest label to this PR? I just double checked and it appears that it isn't needed.

@FiniteReality

This comment has been minimized.

Copy link
Collaborator

commented Oct 19, 2017

I think hacktoberfest labels only apply to issues: PRs are automagically counted.

@foxbot

foxbot approved these changes Oct 19, 2017

@foxbot foxbot merged commit f996338 into discord-net:dev Oct 26, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@Chris-Johnston Chris-Johnston deleted the Chris-Johnston:permissionsOverhaul branch Oct 26, 2017

FiniteReality added a commit to FiniteReality/Discord.Net that referenced this pull request May 5, 2018

Proposed Solution for discord-net#674 Permissions Changes (discord-ne…
…t#743)

* Initial commit of changes. Changed permissions from bitwise index to use bitwise flags instead. Modified relevant methods involved

* Revised enum value naming

* Added FlagsAttribute to ChannelPermission, GuildPermission

* Added comments per Joe4evr suggestion

* Added underlines to hex value digits for readability per Joe4evr suggestion

* updated names to better reflect actual permission names as per SubZero0 suggestion

* fix for 236775c

* Replaced Math.Pow with left shift operator

* Cleaned up the formatting of ChannelPermission and GuildPermission enums to make it easier to read
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.