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

Feature rights #91

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Feature rights #91

wants to merge 2 commits into from

Conversation

rapus95
Copy link
Member

@rapus95 rapus95 commented Jun 30, 2021

This is a first trial on how to implement rights management solely bound to Guild Roles.

@@ -0,0 +1,70 @@
const Functionality = Val
const PATHBASE = "data/permissions/"
Copy link
Member

Choose a reason for hiding this comment

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

Can you use a more specific name that relates to permission?


has_access(guid::Snowflake, member::Member, task::Functionality) = has_access(guid, member.roles, task)

function has_access(guid::Snowflake, roles::Vector{Snowflake}, ::Functionality{Task}) where Task
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it's time to graduate from using Val to an abstract type? So, you can just do T <: AbstractTask.



function load()
isdir(PATHBASE) || return
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should log a warning before returning?

Comment on lines +43 to +49
while !eof(io)
lensym = read(io, UInt8)
lenroles = read(io, UInt8)
sym = Symbol(read(io, lensym))
roles = Set(reinterpret(Snowflake, read(io, lenroles)))
PERMISSIONS[sym] = roles
end
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of building custom SerDes. We are already using StructTypes and JSON3 and that's pretty easy. For example:

function discourse_save(message_id::UInt64, r::DiscourseData)
@info "Saving discourse search data for message_id=$message_id"
path = discourse_file_path(message_id)
write(ensurepath!(path), JSON3.write(r))
end
function discourse_load(message_id::UInt64)
@info "Loading discourse search data for message_id=$message_id"
path = discourse_file_path(message_id)
bytes = read(path)
return JSON3.read(bytes, DiscourseData)
end

If you choose to use custom data types:

# JSON3 struct bindings
StructTypes.StructType(::Type{HoJBot.DiscourseData}) = StructTypes.Struct()
StructTypes.StructType(::Type{HoJBot.DiscoursePost}) = StructTypes.Struct()

end

function grant!(guid::Snowflake, role::Snowflake, ::Functionality{Task}) where Task
guildperms = PERMISSIONS[guid]
Copy link
Member

Choose a reason for hiding this comment

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

Don't shoot me 😄

Suggested change
guildperms = PERMISSIONS[guid]
guild_perms = PERMISSIONS[guid]

function handle_command end

"Entry point for a new observer plugin, triggers on almost all events related to text chat"
function handle_observer end #I suggest renaming handlers to observers since they act on observations
Copy link
Member

Choose a reason for hiding this comment

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

How about handle_event?

Comment on lines +23 to +24
"""GuildID->(Functionality->[permitted roles])"""
const PERMISSIONS = Dict{Snowflake,Dict{Symbol,Set{Snowflake}}}()
Copy link
Member

Choose a reason for hiding this comment

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

Can you move constant definitions to the top of file? See https://github.com/invenia/BlueStyle#global-variables

open(joinpath(dir, string(guid))) do io
fguid = read(io, Snowflake)
if fguid != guid
@info "file conflict, tried to load $guid but got $fguid"
Copy link
Member

Choose a reason for hiding this comment

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

Should be @warn?

@tk3369 tk3369 mentioned this pull request Jul 3, 2021
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.

2 participants