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(interactions): DefaultPermission should not be a bool ptr #1100

Closed
wants to merge 1 commit into from
Closed

fix(interactions): DefaultPermission should not be a bool ptr #1100

wants to merge 1 commit into from

Conversation

Sculas
Copy link

@Sculas Sculas commented Feb 16, 2022

DefaultPermission should not be a bool pointer.

You can no longer pass in true or false, it must be a variable and you need to take the address of it. This absolutely makes no sense.

@FedorLap2006
Copy link
Collaborator

FedorLap2006 commented Feb 17, 2022

Thanks for noticing that! I'm going to close this, because it's intentional.

Pointer to bool allows encoder to omit the field, because nil is going to be considered the empty value, instead of false.
The current behaviour of default_permission requires the field to be omitted (nil), when no default permission should be set. Or to be specified (true, false), when default permission should allow/deny usage of slash command without permission overwrite

Just usage of the variable (i.e. just bool), won't work, because if it would be set to false (or not specified when initialising the struct), encoder would omit it, which should not happen.

@Sculas Sculas deleted the fix/unusual-bool-ptr branch February 17, 2022 11:52
@FedorLap2006
Copy link
Collaborator

FedorLap2006 commented Feb 17, 2022

Additionally, the same idea is used in #1026. It has *float64 fields to allow json encoder to omit them as well, to not set a max/min value.

@FedorLap2006
Copy link
Collaborator

Also, @Lucaskyy, you can read more about this behavious here

@Sculas
Copy link
Author

Sculas commented Feb 17, 2022

Ah yeah you're right, it's inconvenient but since there's no other way to do this in Go we'll have to live with it :)

@FedorLap2006
Copy link
Collaborator

There is a couple of ways, but this one is the easiest, since it requires just one wrapper function.

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

2 participants